1508472 - Clang format breaks comments

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Andrew McCreight [:mccr8]]

This comment:

  // (1) if aAsyncSnowWhiteFreeing is false and nsPurpleBufferEntry::mRefCnt is 0 or
  // (2) if the object's nsXPCOMCycleCollectionParticipant::CanSkip() returns true or
  // (3) if nsPurpleBufferEntry::mRefCnt->IsPurple() is false.

I don't think the numbers are important, so maybe there's a bulleted list that clang format recognizes.

Comment 1

[Andrew McCreight [:mccr8]]

Failing that, an empty comment line between each entry should work.

Comment 2

[Karl Tomlinson (:karlt)]

My guess is that clang-format is only reformatting comments when they need reformatting because they are not conforming to the 80 column limit.

Comment 3

[Mike Hommey [:glandium]]

There are a lot of comments being garbled by clang-format. Would it make sense to exclude comments from reformatting?

Comment 4

[Karl Tomlinson (:karlt)]

ReflowComments:false would certainly help in the cases I've seen.

Perhaps that's not ideal elsewhere, but it would be close what is expected to be seen on an 80 column display.

Comment 5

[Karl Tomlinson (:karlt)]

Perhaps we could CommentPragmas: '.' on the mass changes, to avoid all the corruption, but still enforce 80 columns on new check-ins, where someone has a chance to correct.

Comment 6

[Andrew McCreight [:mccr8]]

Another thing from the same file:
    SliceCC,     /* If a CC is in progress, continue it. Otherwise, start a new one. */

Clang format moves the */ to the next line by itself, which is really ugly. Maybe some text editing can shorten the line.

Comment 7

[(no longer active)]

CCing bz since he was looking at the impact of clang-format on comments as well.

(In reply to Karl Tomlinson (:karlt) from comment #5)
> Perhaps we could CommentPragmas: '.' on the mass changes, to avoid all the
> corruption, but still enforce 80 columns on new check-ins, where someone has
> a chance to correct.

Yes, this may be a good idea.

Comment 8

[Ted Campbell [:tcampbell]]

FYI, I went through js/src to fix comment reflow issues in Bug 1508255.

The approach I used was to clang-format without reflowing comments, make a temporary commit, re-run clang-format reflowing comments and looking at the diff (ignoring whitespace). In the case of js/src that brought the delta down to ~10k which while unpleasant was human workable (though unpleasant).

Clang-format does generate better results when reflowing is allowed, so it is preferable if the fallout is controllable.

> -  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.

One thing to remember is that if your block comments are already 80-cols they will be left alone and your ASCII-art will be unharmed. Documentation that is more unwieldy should probably just use |clang-format off| and not overthink anything.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

I did the same for dom/media: https://hg.mozilla.org/integration/autoland/rev/f70098015fd4

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Clang-format does generate better results when reflowing is allowed

Yeah...  I did an exercise similar to Ted's over the whole tree, though I haven't looked at the diff much yet.  One thing I noticed already, in nsXULElement.h:

-      : mName(nsGkAtoms::
-                  id)  // XXX this is a hack, but names have to have a value
+      : mName(nsGkAtoms::id)  // XXX this is a hack, but names have to have a
+                              // value

The way that got formatted before comment breaking was allowed is rather unfortunate.

Is there a knob where we can enable breaking for comments that start after non-whitespace content on a line, like both these examples, while disabling it for comments that only come after whitespace (like the big block comments we really care about here)?  Or should we just have module owners go through the diffs involved and just manually fix stuff?  The diff for js/src is about 10k, but the one for dom/ is close to 1.5MB, and the whole tree is 13MB...

Comment 11

[Ted Campbell [:tcampbell]]

One idea might be to sort those diffs by the largest + spans to quickly get a list of doc comments that are mangled and put clang-format on them. Could be scripted with some sanity checks on the result. Sounds like there are more problematic patterns than I hoped in the general tree.

Comment 12

[Boris Zbarsky [:bzbarsky]]

That said, of that 13MB, ~4MB is in intl/icu, which I assume we are not formatting, right? And a large chunk of gfx/ is skia and angle.

I should probably go through .clang-format-ignore and get those things out of my tree...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yeah, now I'm down to 5.5MB total.  DOM is still at 1.5MB, but gfx is down to 600KB, and intl to a manageable 17KB.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Zipped-up full-tree diff in case someone wants to look through things

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

1.5MB doesn't sound too bad for DOM. Could we split that to couple of pieces and just review?
I'd be happy to look at some of that.

Comment 16

[Boris Zbarsky [:bzbarsky]]

So I took a closer look at the DOM bits, and a huge chunk of that (~1MB) is dom/media.  And under there, 650KB is dom/media/platforms/ffmpeg and another 60KB is dom/media/webaudio/blink.  Both of which should perhaps parts of them not being formatted at all, right?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Per-directory diffs for dom/* except dom/media

We should perhaps spin this out to a separate bug...

Comment 18

[Andrew McCreight [:mccr8]]

Feel free to repurpose this bug. I don't entirely understand what all is going on here.

Comment 19

[Ted Campbell [:tcampbell]]

Oops, I should have said js/src was 10k-lines so probably roughly the scale of gfx. The approach I took was to have the diff file in one terminal, and then apply fixes as a went to clean tree (with lots of little commits...) and a really fast scan of the results.

Main reasons for manual fixes were:

  - Lists with '-', '1)', ':', etc that prefer leading whitespace on line-wrap
  - Code samples in comments should be manually wrapped to something reasonable looking
  - Large comment block over 80-col. I found blindly applying |clang-format off| was better than trying fix them and complicating the patch.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Oops, I should have said js/src was 10k-lines 

Hmm.  I'm actually seeing a very small diff for js/src after I apply .clang-format-ignore...

That said, I just realized my diffs only include formatting .h and .cpp files, not .c files, in case that matters.

Comment 21

[Ted Campbell [:tcampbell]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #20)
> Hmm.  I'm actually seeing a very small diff for js/src after I apply
> .clang-format-ignore...

Probably depends on what revision. Until yesterday, js/src/.clang-format existed, disabled comment reflow, and use mozilla-style. Most of our changes are currently riding autoland and those changes include removing js/src/.clang-format. We are nearly done our cleanups at this point. A few ignores are still needed for irregexp / js/src/util/Unicode.

Comment 22

[(no longer active)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #16)
> So I took a closer look at the DOM bits, and a huge chunk of that (~1MB) is
> dom/media.  And under there, 650KB is dom/media/platforms/ffmpeg and another
> 60KB is dom/media/webaudio/blink.  Both of which should perhaps parts of
> them not being formatted at all, right?

They're included here: <https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/.clang-format-ignore#51> but I see that not everything under dom/media/platforms/ffmpeg is listed.  Filed bug 1508773.

Comment 23

[Andrew McCreight [:mccr8]]

This isn't quite related to comments, but the way clang-format breaks up long string literals is weird:

-    NS_WARNING(nsPrintfCString("Reentrancy error: some client attempted "
-      "to display a message to the console while in a console listener. "
-      "The following message was discarded: \"%s\"", msg.get()).get());
+        nsPrintfCString(
+            "Reentrancy error: some client attempted "
+            "to display a message to the console while in a console listener. "
+            "The following message was discarded: \"%s\"",

Comment 24

[Andrew McCreight [:mccr8]]

Oh I see, it just isn't reflowing the literals. That actually does make sense.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> They're included here:

Ah, that was after I pulled by tree...

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated full-tree diff

Comment 27

[(no longer active)]

Attachment: Bug 1508472 - Part 1: First batch of comment fix-ups in preparation for the tree reformat

This is a best effort attempt at ensuring that the adverse impact of
reformatting the entire tree over the comments would be minimal.  I've used a
combination of strategies including disabling of formatting, some manual
formatting and some changes to formatting to work around some clang-format
limitations.

Comment 28

[(no longer active)]

I've started to go through the giant diff, trying to ensure that the adverse effect of clang-format on comments is as little as possible.  I'm hoping to put up a few more patches, but this is a super slow process.  I've gone through attachment 9027674 [details] mostly linearly so far...

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb09acfff168
Part 1: First batch of comment fix-ups in preparation for the tree reformat r=sylvestre

Comment 30

[(no longer active)]

Attachment: Bug 1508472 - Part 2: Second batch of comment fix-ups in preparation for the tree reformat

This is a best effort attempt at ensuring that the adverse impact of
reformatting the entire tree over the comments would be minimal.  I've used a
combination of strategies including disabling of formatting, some manual
formatting and some changes to formatting to work around some clang-format
limitations.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11d6688b953f
Part 2: Second batch of comment fix-ups in preparation for the tree reformat r=sylvestre

Comment 32

[Andreea Pavel [:apavel]]

Backed out for build bustages

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=214152790&revision=11d6688b953f800e234162bc8ed5842954de087f

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=214152790&repo=autoland&lineNumber=23090

Backout: https://hg.mozilla.org/integration/autoland/rev/f343188bdcf2a7566fc943bbae4a3ec5e7856c29

Comment 33

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/fb09acfff168

Comment 34

[Andrew McCreight [:mccr8]]

Another pattern I noticed that gets mangled up with clang-format is comments in initializer lists using commas at the start:

For instance, this:
      // Timer thread state may be accessed during GC, so uses of this monitor
      // are not preserved when recording/replaying.
    , mMonitor("TimerEventAllocator", recordreplay::Behavior::DontPreserve)
gets turned into something more like:
        // Timer thread state may be accessed during GC, so uses of this monitor
        // are not preserved when recording/replaying.
        ,
        mMonitor("TimerEventAllocator", recordreplay::Behavior::DontPreserve) {}

I'm fixing a few instances of this I noticed in XPCOM.

Comment 35

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1508472 - Part 3: Third batch of comment fix-ups in preparation for the tree reformat (layout/) r?ehsan

Comment 36

[(no longer active)]

(In reply to Andrew McCreight [:mccr8] from comment #34)
> Another pattern I noticed that gets mangled up with clang-format is comments
> in initializer lists using commas at the start:
> 
> For instance, this:
>       // Timer thread state may be accessed during GC, so uses of this
> monitor
>       // are not preserved when recording/replaying.
>     , mMonitor("TimerEventAllocator", recordreplay::Behavior::DontPreserve)
> gets turned into something more like:
>         // Timer thread state may be accessed during GC, so uses of this
> monitor
>         // are not preserved when recording/replaying.
>         ,
>         mMonitor("TimerEventAllocator",
> recordreplay::Behavior::DontPreserve) {}
> 
> I'm fixing a few instances of this I noticed in XPCOM.

Thanks, much appreciated!  Strangely enough, I have yet to encounter this in code I have been looking at.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2320933cb7bc
Part 2: Second batch of comment fix-ups in preparation for the tree reformat r=sylvestre

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af951294cf96
Part 3: Third batch of comment fix-ups in preparation for the tree reformat (layout/) r=Ehsan

Comment 39

[Cosmin Sabou [:CosminS]]

Backed out for causing build bustages on JobScheduler_posix.cpp

Pushes with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=busted&fromchange=2320933cb7bc8255b7e484df1ef27718154362d5&tochange=193a2a5ad0e6148fa266929af17ff2d897af9f12&selectedJob=214220609

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=214220609&repo=autoland&lineNumber=21901

Backout link: https://hg.mozilla.org/integration/autoland/rev/193a2a5ad0e6148fa266929af17ff2d897af9f12

Comment 40

[Daniel Holbert [:dholbert]]

Looks the backout problem was "error: multi-line comment [-Werror=comment]" build failures due to this chunk of part 2:
  https://hg.mozilla.org/integration/autoland/rev/2320933cb7bc8255b7e484df1ef27718154362d5#l36.2

Presumably that code is destined to be uncommented someday, so we need to be sure it's still valid code while it's commented out. So the trailing slash does need to stay there, assuming we need the linebreak.

Maybe if we rewrite the comment to use /**/ rather //-style comments, the warning would go away?  The warning seems to be to avoid confusing code like that shown in https://stackoverflow.com/questions/7059549/c-multi-line-comments-using-backslash (where the second line of the comment lacks an initial // and so looks like code despite the fact that it's a comment).  There's no such ambiguity for a trailing \ inside of a /**/ comment, so I'll bet the warning wouldn't happen if we made that change.

Comment 41

[(no longer active)]

(In reply to Daniel Holbert [:dholbert] from comment #40)
> Looks the backout problem was "error: multi-line comment [-Werror=comment]"
> build failures due to this chunk of part 2:

Yes.  Off to try this patch goes...

> Presumably that code is destined to be uncommented someday, so we need to be
> sure it's still valid code while it's commented out. So the trailing slash
> does need to stay there, assuming we need the linebreak.
> 
> Maybe if we rewrite the comment to use /**/ rather //-style comments, the
> warning would go away?  The warning seems to be to avoid confusing code like
> that shown in
> https://stackoverflow.com/questions/7059549/c-multi-line-comments-using-
> backslash (where the second line of the comment lacks an initial // and so
> looks like code despite the fact that it's a comment).  There's no such
> ambiguity for a trailing \ inside of a /**/ comment, so I'll bet the warning
> wouldn't happen if we made that change.

I'll try this, and if it doesn't work, I'll go for the simpler option which is just dropping the backslash from the end of the line and letting the person to uncomment the code take care of making sure it's still valid code...

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d8ce84e0107
Part 2: Second batch of comment fix-ups in preparation for the tree reformat r=sylvestre

Comment 43

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2d8ce84e0107

Comment 44

[(no longer active)]

Attachment: Bug 1508472 - Part 4: Fourth batch of comment fix-ups in preparation for the tree reformat

This is a best effort attempt at ensuring that the adverse impact of
reformatting the entire tree over the comments would be minimal.  I've used a
combination of strategies including disabling of formatting, some manual
formatting and some changes to formatting to work around some clang-format
limitations.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4662b6db1b3
Part 4: Fourth batch of comment fix-ups in preparation for the tree reformat r=sylvestre

Comment 46

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b4662b6db1b3

Comment 47

[(no longer active)]

Attachment: Bug 1508472 - Part 5: Fifth batch of comment fix-ups in preparation for the tree reformat

This is a best effort attempt at ensuring that the adverse impact of
reformatting the entire tree over the comments would be minimal. I've used a
combination of strategies including disabling of formatting, some manual
formatting and some changes to formatting to work around some clang-format
limitations.

Comment 48

[(no longer active)]

This is the last patch in this series, and it's green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bb75cdbbe6ddf3ed0a27ab438d500f9798d7dad

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04f0bbf40bf3
Part 5: Fifth batch of comment fix-ups in preparation for the tree reformat r=sylvestre

Comment 50

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/04f0bbf40bf3

Comment 51

[(no longer active)]

Looks like part 3 in these series was backed out once (by mistake) and never got relanded.  :-(  So for the ESR uplifts I won't request uplift for that part.

Comment 52

[(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): These patches only change comments and shouldn't have any impact on the binary code we ship.

String or UUID changes made by this patch: None

Comment 53

[(no longer active)]

Attachment: ESR patch (part 2)

Comment 54

[(no longer active)]

Attachment: ESR patch (part 4)

Comment 55

[(no longer active)]

Attachment: ESR patch (part 5)

Comment 56

[(no longer active)]

Attachment: ESR patch (part 5)

Comment 57

[Boris Zbarsky [:bzbarsky]]

> Looks like part 3 in these series was backed out once (by mistake) and never got relanded.

Does that mean wae ended up with the mass-reformat messing up comment formatting in layout?

Comment 58

[(no longer active)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #57)
> > Looks like part 3 in these series was backed out once (by mistake) and never got relanded.
> 
> Does that mean wae ended up with the mass-reformat messing up comment
> formatting in layout?

Unfortunately, yes.  :-(

Comment 59

[(no longer active)]

(In reply to :Ehsan Akhgari from comment #58)
> (In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #57)
> > > Looks like part 3 in these series was backed out once (by mistake) and never got relanded.
> > 
> > Does that mean wae ended up with the mass-reformat messing up comment
> > formatting in layout?
> 
> Unfortunately, yes.  :-(

(If you have any ideas on how we can fix this, please do share, we could do that in a follow-up.)

Comment 60

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

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

From discussion with ehsan sounds like we intend to stay consistent with m-c and file a follow up bug for later. 

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

Comment 61

[Boris Zbarsky [:bzbarsky]]

> (If you have any ideas on how we can fix this, please do share, we could do that in a follow-up.)

Well, we need to find the comments that ended up broken (i.e. where whitespace was significant) and fix them, no?

So basically go through whatever process created part 3 and do it over?

Comment 62

[(no longer active)]

karlt suggested trying to rebase that patch on trunk...

Comment 63

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/9cc0f3731810
https://hg.mozilla.org/releases/mozilla-esr60/rev/199a551324f0
https://hg.mozilla.org/releases/mozilla-esr60/rev/138b3a6f6c67
https://hg.mozilla.org/releases/mozilla-esr60/rev/7967cfc328de

Comment 64

[Cameron McCormack (:heycam)]

(In reply to :Ehsan Akhgari from comment #62)
> karlt suggested trying to rebase that patch on trunk...

As it looks like Karl discovered, I went through layout/ to fix up comments in bug 1511854.

Comment 65

[(no longer active)]

(In reply to Cameron McCormack (:heycam) from comment #64)
> (In reply to :Ehsan Akhgari from comment #62)
> > karlt suggested trying to rebase that patch on trunk...
> 
> As it looks like Karl discovered, I went through layout/ to fix up comments
> in bug 1511854.

Thanks so much!