1508065 - JS_PUBLIC_API and JS_FRIEND_API get mangled by clang-format

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Ted Campbell [:tcampbell]]

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.

Comment 1

[Tooru Fujisawa [:arai]]

simplest solution would be just remove arguments part and align with other MOZ_something

> extern JS_PUBLIC_API bool

Comment 2

[Jan de Mooij [:jandem]]

I like arai's suggestion. I'll look into it today.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: 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

Comment 4

[Ted Campbell [:tcampbell]]

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?

Comment 5

[Jason Orendorff [:jorendorff]]

Great. Thank you!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/4b7c53151158

Comment 8

[(no longer active)]

Attachment: ESR patch

[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

Comment 9

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 9030977 [details] [diff] [review]
ESR patch

OK for uplift to ESR60 as part of the clang-format project.

Comment 10

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/374668f081a4