Clang format breaks comments

RESOLVED FIXED in Firefox -esr60

Status

defect
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: mccr8, Assigned: Ehsan)

Tracking

(Blocks 1 bug)

unspecified
mozilla65
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 fixed, firefox65 fixed)

Details

Attachments

(10 attachments, 3 obsolete attachments)

202.48 KB, application/zip
Details
911.01 KB, application/zip
Details
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
82.07 KB, patch
Details | Diff | Splinter Review
100.08 KB, patch
Details | Diff | Splinter Review
144.83 KB, patch
Details | Diff | Splinter Review
184.94 KB, patch
Details | Diff | Splinter Review
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.
Failing that, an empty comment line between each entry should work.
My guess is that clang-format is only reformatting comments when they need reformatting because they are not conforming to the 80 column limit.
There are a lot of comments being garbled by clang-format. Would it make sense to exclude comments from reformatting?
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.
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.
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.
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.
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.
> 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...
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.
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...
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.
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.
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?
Flags: needinfo?(ehsan)
We should perhaps spin this out to a separate bug...
Feel free to repurpose this bug. I don't entirely understand what all is going on here.
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.
Assignee: continuation → nobody
Component: XPCOM → General
Summary: Clang format breaks numbered comment list in RemoveSkippable → Clang format breaks comments
Component: General → Lint and Formatting
Product: Core → Firefox Build System
> 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.
(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.
(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.
Flags: needinfo?(ehsan)
See Also: → 1508298
> They're included here:

Ah, that was after I pulled by tree...
Attachment #9026451 - Attachment is obsolete: true
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.
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...
Keywords: leave-open
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb09acfff168
Part 1: First batch of comment fix-ups in preparation for the tree reformat r=sylvestre
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.
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11d6688b953f
Part 2: Second batch of comment fix-ups in preparation for the tree reformat r=sylvestre
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.
Attachment #9028040 - Attachment description: Bug 1508472 - Part 3: Third batch of comment fix-ups in preparation for the tree reformat (layout/) r?andi → Bug 1508472 - Part 3: Third batch of comment fix-ups in preparation for the tree reformat (layout/) r?ehsan
(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.
Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2320933cb7bc
Part 2: Second batch of comment fix-ups in preparation for the tree reformat r=sylvestre
Pushed by eakhgari@mozilla.com:
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
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.
(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...
Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d8ce84e0107
Part 2: Second batch of comment fix-ups in preparation for the tree reformat r=sylvestre
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.
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4662b6db1b3
Part 4: Fourth batch of comment fix-ups in preparation for the tree reformat r=sylvestre
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.
This is the last patch in this series, and it's green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bb75cdbbe6ddf3ed0a27ab438d500f9798d7dad
Keywords: leave-open
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/04f0bbf40bf3
Part 5: Fifth batch of comment fix-ups in preparation for the tree reformat r=sylvestre
https://hg.mozilla.org/mozilla-central/rev/04f0bbf40bf3
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
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.
[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
Attachment #9030999 - Flags: approval-mozilla-esr60?
Attachment #9031000 - Flags: approval-mozilla-esr60?
Attachment #9031001 - Flags: approval-mozilla-esr60?
Posted patch ESR patch (part 5) (obsolete) — Splinter Review
Attachment #9031002 - Flags: approval-mozilla-esr60?
Attachment #9031002 - Attachment is obsolete: true
Attachment #9031002 - Flags: approval-mozilla-esr60?
Attachment #9031003 - Flags: approval-mozilla-esr60?
> 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?
Flags: needinfo?(ehsan)
(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.  :-(
Flags: needinfo?(ehsan)
(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 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.
Attachment #9030999 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Attachment #9031000 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Attachment #9031001 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Attachment #9031003 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
> (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?
karlt suggested trying to rebase that patch on trunk...
Flags: needinfo?(ehsan)
Depends on: 1515556
Flags: needinfo?(ehsan)
Attachment #9028040 - Attachment is obsolete: true
(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.
(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!
Target Milestone: Firefox 65 → mozilla65
You need to log in before you can comment on or make changes to this bug.