FOR_EACH_* macros in js/ get mangled by clang-format

RESOLVED FIXED in Firefox -esr60

Status

()

enhancement
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: tcampbell, Assigned: tcampbell)

Tracking

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 fixed, firefox65 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Assignee

Description

6 months ago
We use this macro pattern a lot in js/ and should probably wrap them with // clang-format off/on blocks.

This seems like precedent for tabular data: https://hg.mozilla.org/mozilla-central/rev/7807742373e1

A question I still have is once 4-space tabs become 2-space tabs, will we change the formatting in this blocks? The reasonable compromise would be to leave it alone until the next time someone does something like reformat the width of a column.

This query is a good starting point. Some of these become crazy in under clang-format (eg JS_FOR_PROTOTYPES).
https://searchfox.org/mozilla-central/search?q=%23define.*FOR_EACH&case=true&regexp=true&path=js
Assignee

Comment 1

6 months ago
FOR_EACH_SIZE in js/public/MemoryMetrics.h are particularly broken.
Assignee

Updated

6 months ago
Assignee: nobody → tcampbell
Assignee

Comment 2

6 months ago
Aha! If we use uppercase macro arguments for re-expansion in these lists, the formatting is actually sane.
Assignee

Updated

6 months ago
Depends on: 1508180
Assignee

Comment 3

6 months ago
Moved uppercasing to Bug 1508180 since it touches different components. This bug will instead focus on the cases that still need |clang-format off|.
Assignee

Comment 4

6 months ago
Add JS_FOR_PROTOTYPES_ macro that takes REAL_IF_SAB, etc helpers to
handle conditional proto keys. This is easier to read and avoids macro
expansion issues confusing clang-format.
Assignee

Comment 5

6 months ago
Protect tabular macros or struct initializers that can mangled by
clang-format.

Depends on D12238

Comment 6

6 months ago
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a50839e59152
Simplify JS_FOR_PROTOTYPES. r=jandem
https://hg.mozilla.org/integration/autoland/rev/1963c82b5abf
Use clang-format off in parts of js/ r=jandem

Comment 7

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a50839e59152
https://hg.mozilla.org/mozilla-central/rev/1963c82b5abf
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Posted patch ESR patch (part 1) (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): This is reworking a C++ macro, shouldn't be making a functional change.  The patch has also baked on trunk for a while.

String or UUID changes made by this patch: None
Attachment #9030756 - Flags: review?(jdemooij)
Attachment #9030756 - Flags: approval-mozilla-esr60?
Attachment #9030756 - Attachment is patch: true
Attachment #9030756 - Attachment mime type: application/octet-stream → text/plain
[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 #9030759 - Flags: approval-mozilla-esr60?
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: See comment 8 (attached the wrong file!)

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 #9030770 - Flags: review?(jdemooij)
Attachment #9030770 - Flags: approval-mozilla-esr60?
Attachment #9030756 - Attachment is obsolete: true
Attachment #9030756 - Flags: review?(jdemooij)
Attachment #9030756 - Flags: approval-mozilla-esr60?
Attachment #9030770 - Attachment is patch: true
Attachment #9030770 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 9030770 [details] [diff] [review]
ESR patch (part 1)

Review of attachment 9030770 [details] [diff] [review]:
-----------------------------------------------------------------

Same patch just with SIMD instead of BigInt AFAICT, LGTM.
Attachment #9030770 - Flags: review?(jdemooij) → review+
Comment on attachment 9030759 [details] [diff] [review]
ESR patch (part 2)

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