Closed Bug 1508095 Opened 9 months ago Closed 9 months ago

Fix clang-format around JS_FN_HELP

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- fixed
firefox65 --- fixed

People

(Reporter: tcampbell, Assigned: jandem)

References

Details

Attachments

(2 files)

These JS_FN_HELP macro uses have their own line breaking for the messages that gets mangled by clang-format.

These probably should be |clang-format off| for line breaking, but probably could stand to have their formatting manually fixed to be more consistent (eg. add missing leading whitespace). Similarly I wonder if parameter 5 should be consistently on the first line.
I wonder if this (from clang-format):

------
  JS_FN_HELP("setSavedStacksRNGState", SetSavedStacksRNGState, 1, 0,
             "setSavedStacksRNGState(seed)",
             "  Set this compartment's SavedStacks' RNG state.\n"),
------

is actually nicer than what we have now:

------
    JS_FN_HELP("setSavedStacksRNGState", SetSavedStacksRNGState, 1, 0,
"setSavedStacksRNGState(seed)",
"  Set this compartment's SavedStacks' RNG state.\n"),
------

It might make sense to use the first style everywhere and ensure 80 cols so clang-format won't touch it?
It might be nice to use C++11 raw string literals so we don't have to add \n ourselves. Something like:

R"(
  Run the garbage collector. When obj is given, GC only its zone.
  If 'zone' is given, GC any zones that were scheduled for
  GC via schedulegc.
  If 'shrinking' is passed as the optional second argument, perform a
  shrinking GC rather than a normal GC.)"),

Unfortunately this adds a leading \n but maybe we can skip that somehow.
I'd also support just a |clang-format off| to get it off our plate. In the future we can change how \n are used in string too to make it a little prettier.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07b82a902b85
Add clang-format off/on annotations for JSFunctionSpecWithHelp arrays. r=tcampbell
https://hg.mozilla.org/mozilla-central/rev/07b82a902b85
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Attached patch yyyyyySplinter Review
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is required for easier backporting of patches after the reformatting of ESR using clang-format.

User impact if declined: Declining this will negatively impact our developers' ability to easily backport their patches to ESR in the future.

Fix Landed on Version: 65

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): comment only

String or UUID changes made by this patch: None
Attachment #9030795 - Flags: approval-mozilla-esr60?
Attachment #9030795 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Comment on attachment 9030795 [details] [diff] [review]
yyyyyy

OK for uplift to ESR60 as part of the clang-format project.
You need to log in before you can comment on or make changes to this bug.