Allow clang-format to reflow js/src comments

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

(16 attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
30.01 KB, patch
Details | Diff | Splinter Review
51.36 KB, patch
Details | Diff | Splinter Review
46.38 KB, patch
Details | Diff | Splinter Review
35.84 KB, patch
Details | Diff | Splinter Review
55.33 KB, patch
Details | Diff | Splinter Review
11.81 KB, patch
Details | Diff | Splinter Review
1020 bytes, patch
Details | Diff | Splinter Review
1.61 KB, patch
Details | Diff | Splinter Review
Assignee

Description

6 months ago
While we currently have a comment-exclusion in js/src/.clang-format, there aren't too many things that need to be fixed so it might be worth doing so in this same round of formatting and eliminating js/src/.clang-format.

I spotted roughly a dozen diagrams that can mangled and need |clang-format off|. Searching for '-----' is a good start to find most of them.

Another small source of silliness is '/*** Section Title ****************************************************/' that triggers a funny line-break. These can be fixed too.
Assignee

Comment 1

6 months ago
This is worth just doing. I'll put up a patch series.
Assignee: nobody → tcampbell
Summary: Guard ascii diagrams in js/src from clang-format → Allow clang-format to reflow js/src comments
Assignee

Comment 2

6 months ago
To evaluate throwing this option I ran clang-format with the current options, make a commit, changed the options to allow formatting, and then re-ran clang-format. The result is a diff that shows the incremental effect of the option without drowning in the million-line clang-format diff.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b36137f3854bff31326afcd93393aca59adf6004
Rev with delta: https://hg.mozilla.org/try/rev/5407ed19d50b6344b78bb08b82095174eca4cbb1
Assignee

Comment 3

6 months ago
The remaining problems should be handled with the following bugs:
 - Bug 1507887 (irregexp clang-format)
 - Bug 1508176 (unicode clang-format)
 - Bug 1508178 (BytecodeEmitter stack comments)
Assignee

Comment 4

6 months ago
These comments get mangled by clang-format so use |clang-format off|
around them for now. In the future they can be rewritten if desired.
Assignee

Comment 6

6 months ago
These cause clang-format to generate better results when reflowing
comments.

Depends on D12386
Assignee

Comment 7

6 months ago
These also help the clang-format result but are more subjective.

Depends on D12387
Assignee

Comment 8

6 months ago
This generates much better results for clang-format.

Depends on D12388
Assignee

Comment 9

6 months ago
Depends on D12389
Assignee

Comment 10

6 months ago
Previous patches on this bug fix sources of bad comment reflow. This
patch changes the option in js/src/.clang-format. Doing this puts
SpiderMonkey in-line with Gecko and generates better results from
clang-format.

Depends on D12390
Assignee

Comment 11

6 months ago
The style options now match, so remove js/src/.clang-format \o/

Depends on D12391
Assignee

Comment 12

6 months ago
Example where reflowing comments gives better results for code:

> -  MOZ_TRY(readConst(
> -      COMPRESSION_IDENTITY));  // For the moment, we only support identity compression.
> +  MOZ_TRY(readConst(COMPRESSION_IDENTITY));  // For the moment, we only support
> +                                             // identity compression.
Depends on: 1508605

Comment 13

6 months ago
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c86b9e22b393
Use |clang-format off| for some js/src comments r=jandem
https://hg.mozilla.org/integration/autoland/rev/cf2b7ca307b6
Truncate '***' lines in js/src comments r=jandem
https://hg.mozilla.org/integration/autoland/rev/ba2da67c9e7e
Minor formatting changes in js/src r=jandem
https://hg.mozilla.org/integration/autoland/rev/32eadea53faa
More formatting changes in js/src r=jandem
https://hg.mozilla.org/integration/autoland/rev/5219139e6d0f
Reformat comments in js/src/Stream.cpp r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/fa3eaa0d2cde
Wrap ES Spec reference comments r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/d437391bafa1
Allow clang-format to reflow comments in js/src r=jandem,arai
https://hg.mozilla.org/integration/autoland/rev/2d8d64aff05a
Merge js/src/.clang-format into top-level r=sylvestre
Assignee

Comment 14

6 months ago
https://hg.mozilla.org/try/rev/9a1d431b2cbb5af6705fb525e31b269be7fa1e99

Here is current delta after this landed for all js/. The js/public directory looks in pretty good shape. We can do small fixes if we like.

Comment 16

5 months ago
[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 change

String or UUID changes made by this patch: None
Attachment #9030990 - Flags: approval-mozilla-esr60?

Comment 17

5 months ago
Attachment #9030991 - Flags: approval-mozilla-esr60?

Comment 18

5 months ago
Attachment #9030992 - Flags: approval-mozilla-esr60?

Comment 19

5 months ago
Attachment #9030993 - Flags: approval-mozilla-esr60?

Updated

5 months ago
Attachment #9030993 - Attachment description: ESR patch → ESR patch (part 4)

Comment 20

5 months ago
Attachment #9030994 - Flags: approval-mozilla-esr60?

Comment 21

5 months ago
Attachment #9030995 - Flags: approval-mozilla-esr60?

Comment 22

5 months ago
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

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): This is NPOTB.

String or UUID changes made by this patch:
Attachment #9030996 - Flags: approval-mozilla-esr60?

Comment 23

5 months ago
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

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): This is NPOTB.

String or UUID changes made by this patch:
Attachment #9030997 - Flags: approval-mozilla-esr60?
Comment on attachment 9030990 [details] [diff] [review]
ESR patch (part 1)

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