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

RESOLVED FIXED in Firefox -esr60

Status

()

defect
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: tcampbell, Assigned: jandem)

Tracking

unspecified
mozilla65
Points:
---

Firefox Tracking Flags

(firefox-esr60 fixed, firefox65 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 months ago
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.
(Reporter)

Updated

6 months ago
Assignee: nobody → tcampbell
(Reporter)

Comment 1

6 months ago
Not currently working on this. We still need a solution before re-format though.
Assignee: tcampbell → nobody
Summary: Make js/src/Interpretter.cpp clang-format friendly → Make js/src/Interpreter.cpp clang-format friendly
(Assignee)

Comment 2

6 months ago
(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)
(Assignee)

Comment 3

6 months ago
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).
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED

Comment 5

6 months ago
Pushed by jdemooij@mozilla.com:
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

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0833d96f563a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Posted patch ESR patch (obsolete) — Splinter 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): only adding braces

String or UUID changes made by this patch: None
Attachment #9030797 - Flags: approval-mozilla-esr60?
Posted patch ESR patchSplinter Review
[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:
Attachment #9030797 - Attachment is obsolete: true
Attachment #9030797 - Flags: approval-mozilla-esr60?
Attachment #9030807 - Flags: approval-mozilla-esr60?
Comment on attachment 9030807 [details] [diff] [review]
ESR patch

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