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.

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

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

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.

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

> They're included here: Ah, that was after I pulled by tree...

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

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

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

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.

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

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

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

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

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

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

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

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

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

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

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

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

(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!