1508106 - Make js/src/Interpreter.cpp clang-format friendly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ted Campbell [:tcampbell]]

The CASE(x) / END_CASE(x) blocks get mangled with clang-format. One option is to consider adding to to .clang-format |BeginMacroBlock| conditions, and another (I think preferable) solution is to enforce { } for interpreter case bodies.

Comment 1

[Ted Campbell [:tcampbell]]

Not currently working on this. We still need a solution before re-format though.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Ted Campbell [:tcampbell] from comment #0)
> The CASE(x) / END_CASE(x) blocks get mangled with clang-format. One option
> is to consider adding to to .clang-format |BeginMacroBlock| conditions

This doesn't work because not every CASE has a matching END_CASE (clang-format will enter a new indentation level when it sees a new CASE). Maybe fixable somehow but not easily. We would also have to rename CASE to JS_INTERP_CASE or something because CASE is used for other macros in the tree.

> and
> another (I think preferable) solution is to enforce { } for interpreter case
> bodies.

This seems to work. Clang still indents the CASE/END_CASE macros and puts the "{" after the CASE() but that's probably fine. If the interpreter project works out then we can hopefully simplify the C++ interpreter and start using "switch JSOP_FOO:" instead of CASE(JSOP_FOO) at some point and clang-format's style would make a lot of sense then.

A bit annoying is that for some (smaller) cases it does:

  CASE(JSOP_INT32) { PUSH_INT32(GET_INT32(REGS.pc)); }
  END_CASE(JSOP_INT32)

Comment 3

[Jan de Mooij [:jandem]]

Things look pretty reasonable I think after adding {} to non-empty CASE/END_CASE blocks that don't have them yet (most of the cases have braces already).

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1508106 - Add {} to non-empty CASE/END_CASE blocks in Interpreter.cpp that don't have them yet to make clang-format work better. r?tcampbell

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0833d96f563a
Add {} to non-empty CASE/END_CASE blocks in Interpreter.cpp that don't have them yet to make clang-format work better. r=tcampbell

Comment 6

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/0833d96f563a

Comment 7

[(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): only adding braces

String or UUID changes made by this patch: None

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: See comment 7

User impact if declined: 

Fix Landed on Version: 

Risk to taking this patch: Low

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

String or UUID changes made by this patch:

Comment 9

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 9030807 [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/0c8853a8ca8a