1508255 - Allow clang-format to reflow js/src comments

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Ted Campbell [:tcampbell]]

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.

Comment 1

[Ted Campbell [:tcampbell]]

This is worth just doing. I'll put up a patch series.

Comment 2

[Ted Campbell [:tcampbell]]

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

Comment 3

[Ted Campbell [:tcampbell]]

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)

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: Bug 1508255 - Use |clang-format off| for some js/src comments

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.

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: Bug 1508255 - Truncate '***' lines in js/src comments

Depends on D12385

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: Bug 1508255 - Minor formatting changes in js/src

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

Depends on D12386

Comment 7

[Ted Campbell [:tcampbell]]

Attachment: Bug 1508255 - More formatting changes in js/src

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

Depends on D12387

Comment 8

[Ted Campbell [:tcampbell]]

Attachment: Bug 1508255 - Reformat comments in js/src/Stream.cpp

This generates much better results for clang-format.

Depends on D12388

Comment 9

[Ted Campbell [:tcampbell]]

Attachment: Bug 1508255 - Wrap ES Spec reference comments

Depends on D12389

Comment 10

[Ted Campbell [:tcampbell]]

Attachment: Bug 1508255 - Allow clang-format to reflow comments in js/src

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

Comment 11

[Ted Campbell [:tcampbell]]

Attachment: Bug 1508255 - Merge js/src/.clang-format into top-level

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

Depends on D12391

Comment 12

[Ted Campbell [:tcampbell]]

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.

Comment 13

[Pulsebot]

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

Comment 14

[Ted Campbell [:tcampbell]]

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 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/c86b9e22b393
https://hg.mozilla.org/mozilla-central/rev/cf2b7ca307b6
https://hg.mozilla.org/mozilla-central/rev/ba2da67c9e7e
https://hg.mozilla.org/mozilla-central/rev/32eadea53faa
https://hg.mozilla.org/mozilla-central/rev/5219139e6d0f
https://hg.mozilla.org/mozilla-central/rev/fa3eaa0d2cde
https://hg.mozilla.org/mozilla-central/rev/d437391bafa1
https://hg.mozilla.org/mozilla-central/rev/2d8d64aff05a

Comment 16

[(no longer active)]

Attachment: ESR patch (part 1)

[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

Comment 17

[(no longer active)]

Attachment: ESR patch (part 2)

Comment 18

[(no longer active)]

Attachment: ESR patch (part 3)

Comment 19

[(no longer active)]

Attachment: ESR patch (part 4)

Comment 20

[(no longer active)]

Attachment: ESR patch (part 5)

Comment 21

[(no longer active)]

Attachment: ESR patch (part 6)

Comment 22

[(no longer active)]

Attachment: ESR patch (part 7)

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

Comment 23

[(no longer active)]

Attachment: ESR patch (part 8)

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

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9030990 [details] [diff] [review]
ESR patch (part 1)

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

Comment 25

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/3eb43538e91a
https://hg.mozilla.org/releases/mozilla-esr60/rev/517302dcd9e1
https://hg.mozilla.org/releases/mozilla-esr60/rev/5ca3611ae290
https://hg.mozilla.org/releases/mozilla-esr60/rev/9c97df04bbf8
https://hg.mozilla.org/releases/mozilla-esr60/rev/11eccbaa9095
https://hg.mozilla.org/releases/mozilla-esr60/rev/bdad97bebfdf
https://hg.mozilla.org/releases/mozilla-esr60/rev/a8d61a880677
https://hg.mozilla.org/releases/mozilla-esr60/rev/879fa39a3f5a