Closed Bug 1508065 Opened 10 months ago Closed 10 months ago

JS_PUBLIC_API and JS_FRIEND_API get mangled by clang-format

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

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

People

(Reporter: tcampbell, Assigned: jandem)

References

Details

Attachments

(2 files)

Under the clang-format proposal, JS_PUBLIC_API seems to confuse clang. 

>  extern JS_PUBLIC_API(bool)
> -JS_ExecuteScript(JSContext* cx, JS::Handle<JSScript*> script);
> +    JS_ExecuteScript(JSContext* cx, JS::Handle<JSScript*> script);

We should determine if there is anything we should do here or if this is expected and we live with it.
simplest solution would be just remove arguments part and align with other MOZ_something

> extern JS_PUBLIC_API bool
Summary: JS_PUBLIC_API gets mangled by clang-format → JS_PUBLIC_API and JS_FRIEND_API get mangled by clang-format
I like arai's suggestion. I'll look into it today.
Flags: needinfo?(jdemooij)
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Comment on attachment 9026028 [details]
Bug 1508065 - Change JS_PUBLIC_{API,DATA} and JS_FRIEND_{API,DATA} to be more like similar macros to avoid confusing clang-format. r?tcampbell

Jason, any opinions on this approach?
Attachment #9026028 - Flags: feedback?(jorendorff)
Attachment #9026028 - Flags: feedback?(jorendorff) → feedback+
Great. Thank you!
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4b7c53151158
Change JS_PUBLIC_{API,DATA} and JS_FRIEND_{API,DATA} to be more like similar macros to avoid confusing clang-format. r=tcampbell
https://hg.mozilla.org/mozilla-central/rev/4b7c53151158
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Attached patch ESR patchSplinter 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): This just reworks a C++ macro, and shouldn't change the generated code in our binaries in any way.  It's been baking on trunk for quite a while now.

The patch is generated by a search and replace in the js/ directory using some regex patterns.

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

OK for uplift to ESR60 as part of the clang-format project.
Attachment #9030977 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.