Proposal: turning on clang format for all the repo

[John Abd-El-Malek]

Since many directories already enforce this through presubmit checks, it seems like developers have mostly voted in favor of this. So how about we move this presumbit check to src/PRESUBMIT?

For the record, these top level directories already enforce it:

apps

ash

base

cc

chromeos

device

ios

media

net

webkit (not strictly top level but one of the biggest directories and might be moved anyways)

v8

most of extensions

14 subdirs of components

14 subdirs of content

12 subdirs of chrome

3 subdirs of ui

[Daniel Cheng]

Sounds good to me!

Daniel

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Luis Hector]

+1! I was about to add it to all the arc-related directories.

[Tommy Nyquist]

+1

[James Cook]

+1

[Nico Weber]

+1

[Brett Wilson]

I used to be opposed to this but I've been worn down and have no opinion any more.

Brett

[Avi Drissman]

I'm mostly in favor, but there are still cases, particularly with data tables, where hand-formatting is much clearer. For example, something like:

[0,   0, 0,   0]

[0, 255, 0, 255]

[0,   0, 0,   0]

where clang-format will collapse everything. If it's OK to  // clang-format off things like that, where it's not a bug in clang-format but that external semantics make it suboptimal, I'd be on board.

Avi

[Dan Beam]

+1

[John Abd-El-Malek]

On Wed, Feb 8, 2017 at 10:54 AM, Avi Drissman <a...@chromium.org> wrote:

I'm mostly in favor, but there are still cases, particularly with data tables, where hand-formatting is much clearer. For example, something like:

[0,   0, 0,   0]

[0, 255, 0, 255]

[0,   0, 0,   0]

where clang-format will collapse everything. If it's OK to  // clang-format off things like that, where it's not a bug in clang-format but that external semantics make it suboptimal, I'd be on board.

It's a presubmit check, so in cases like this I think it seems reasonable to undo the formatting.

Avi

On Wed, Feb 8, 2017 at 1:48 PM, Brett Wilson <bre...@chromium.org> wrote:

I used to be opposed to this but I've been worn down and have no opinion any more.

I used to be as well. There are still things the plugin does that I think are odd, but like the google style guide (cough 80 chars cough), I've come to appreciate that having less things to worry and nit about is better.

[John Abd-El-Malek]

Also, if there's no unaddressed opposition by end of day I'll send a cl to do this.

[dan...@chromium.org]

On Wed, Feb 8, 2017 at 1:59 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Wed, Feb 8, 2017 at 10:54 AM, Avi Drissman <a...@chromium.org> wrote:

I'm mostly in favor, but there are still cases, particularly with data tables, where hand-formatting is much clearer. For example, something like:

[0,   0, 0,   0]

[0, 255, 0, 255]

[0,   0, 0,   0]

where clang-format will collapse everything. If it's OK to  // clang-format off things like that, where it's not a bug in clang-format but that external semantics make it suboptimal, I'd be on board.

It's a presubmit check, so in cases like this I think it seems reasonable to undo the formatting.

In general, //clang-format off is better than undoing it manually, else the next person to touch the code has to deal with the same problems. I agree that it should be fine to sparingly use //clang-format off for things that you actually don't want formatted ever.

[Gabriel Charette]

+1, I have a script that does git cl format && git add -up to curate the changes it makes but I don't remember the last time I rejected one and would very much welcome one less thing to think about :)

[Dan Beam]

On Wed, Feb 8, 2017 at 10:55 AM, Dan Beam <db...@chromium.org> wrote:

+1

I highly recommend locally running clang-format and analyzing gaps before turning on checks (if at all feasible, maybe even just to understand what issues we'll hit).

Backstory: I dropped the JS linter in favor of clang-format.  In this process, I found some issues by running clang-format on many .js files in chrome (mainly ASI and GRIT's <if/include>).  I consulted folks working on clang-format and ended up fixing some issues and tweaking the upstream clang-format -style=Chromium before turning on more checks.  I just reallly didn't want these problems to come up one-by-one, likely caught by others, at possibly inopportune times.  I skipped blink's layout tests, for example, got CC'd on a bug found there.

-- Dan

[Nico Weber]

On Wed, Feb 8, 2017 at 3:43 PM, Dan Beam <db...@chromium.org> wrote:

On Wed, Feb 8, 2017 at 10:55 AM, Dan Beam <db...@chromium.org> wrote:

+1

I highly recommend locally running clang-format and analyzing gaps before turning on checks (if at all feasible, maybe even just to understand what issues we'll hit).

Backstory: I dropped the JS linter in favor of clang-format.  In this process, I found some issues by running clang-format on many .js files in chrome (mainly ASI and GRIT's <if/include>).  I consulted folks working on clang-format and ended up fixing some issues and tweaking the upstream clang-format -style=Chromium before turning on more checks.  I just reallly didn't want these problems to come up one-by-one, likely caught by others, at possibly inopportune times.  I skipped blink's layout tests, for example, got CC'd on a bug found there.

We've extensively used clang-format on cc files for years. We've formatted all of blink with it. I've run it on some large-ish dirs locally. I think it'll be fine.

(js support in clang-format on the other hand is pretty new and wasn't used anywhere before.)

[Nigel Tao]

On Thu, Feb 9, 2017 at 5:54 AM, Avi Drissman <a...@chromium.org> wrote:
> I'm mostly in favor, but there are still cases, particularly with data
> tables, where hand-formatting is much clearer. For example, something like:
>
> [0, 0, 0, 0]
> [0, 255, 0, 255]
> [0, 0, 0, 0]

It's drifting off topic, but for this particular example, you could
write it like:

[0x00, 0x00, 0x00, 0x00]
[0x00, 0xff, 0x00, 0xff]
[0x00, 0x00, 0x00, 0x00]

[Mark Mentovai]

I don’t think that clang-format is ready for us to enforce in this way. I’m impressed with the progress that it’s made, and it’s much better than the last time I looked at it, but I still find myself fighting it fairly regularly. I don’t doubt that if they keep up with improvements, it’ll reach the point where we can start enforcing it more widely.

I ran clang-format in Crashpad and found a number of things that it did that are no better and no worse than what was there before. It did a few things that were clearly better too, but I also found it doing some things that were obviously wrong. Some patterns of bad behavior recur frequently.

I put these notes together with an eye toward providing constructive feedback on why I don’t think we can do this now. I’ve sorted these in mostly descending order of severity. Some of these problems may be specific to the local styles that we use in Crashpad. Running it over any other arbitrary directory may not highlight these quirks, and may reveal others.

Comment formatting before preprocessor #directives is wrong. 1, 2, 3, 4, 5, 6, 7, 8, 9, 10

In-line comments that overflow onto a new line are reformatted badly. 1, 2

It doesn’t seem to understand all preprocessor directives, like #__has_include_next, and can break their contents. 1, 2

As is commonly mentioned, it’s no good with data tables, which would need to be encased in // clang-format off/on. 1, 2, and a particularly egregious example where it mixes in extra spaces for no good reason, 3

Similar to data tables, sometimes humans insert line breaks into expressions for semantic purposes that clang-format is oblivious to, and these would need //clang-format off/on. It would be nice if it made a stronger effort to not modify not-incorrect code in such cases. 1, 2, 3, 4, 5, 6

Similarly, it’s not great with long string literals, which would also need // clang-format off/on. 1, 2, 3

This thing that it does with data tables just means that whenever someone comes along and wants to insert a new element in the middle, or change an existing element so that the column widths need to change, the whole table has to move around, along with blame. 1, 2, 3

It’s also quirky inside macro definitions. It doesn’t seem to understand that a UTF-8 sequence should count as one column (here) and its lack of context makes it do strange things gratuitously (here). It seems to enforce different style rules in macros than it does elsewhere (here and several other examples in that file, related to the bit-wise & operator, but oddly not on line 218).

Other interactions with preprocessor #directives seem wrong. Here, it formats the preprocessor-free arrays at lines 309 and 316 with a +4 indent for elements. The array at line 325, which has some preprocessor stuff going on, is inconsistent in that its elements are given a +2 indent.

Sometimes, I can’t tell why it’s doing what it’s doing. It seems to favor reducing the number of lines that code takes in many cases, so it’s puzzling here when it lumps as much as it can onto line 83 and then leaves lines 84 and 85 virtually empty by comparison (similar). It’s also puzzling in these cases (1, 2, 3, 4, 5, 6, 7, 8, 9, 10) where it takes two style-legal lines and turns them into three (here’s a slightly different pattern: 1, 2, 3, 4, 5, 6).

Finally, a personal nit: when I have a for (a; b; c) that breaks onto more than one line, I like to split it up so that a, b, and c each get their own line, making it easier to find each part of the loop control. clang-format strives to squeeze these onto two lines in any way it can. 1, 2, 3, 4, 5, 6, 7, 8

I encourage others to do as Dan suggested and run clang-format on their favorite directories to see how it handles their code too. Some of these problems that I found are effectively stop-shippers, but I have confidence that the most serious ones could be addressed. That would make me much more comfortable with this proposal.

On Wed, Feb 8, 2017 at 3:43 PM, Dan Beam <db...@chromium.org> wrote:

[Nigel Tao]

On Thu, Feb 9, 2017 at 1:25 PM, Mark Mentovai <ma...@chromium.org> wrote:
> As is commonly mentioned, it’s no good with data tables, which would need to
> be encased in // clang-format off/on. 1, 2, and a particularly egregious
> example where it mixes in extra spaces for no good reason, 3

I'm drifting off topic again, but I was curious about this
particularly egregious example
(https://chromium-review.googlesource.com/c/439744/1/snapshot/mac/system_snapshot_mac.cc#364)
and it appears to me to be inserting extra spaces so that the columns
line up.

That particular example might align better, post clang-format, if the
sequence was "+0, +1, -1, +2, etc." instead of "0, 1, -1, 2, etc".

[Daniel Cheng]

Comments inline. I will say that it seems like a lot of the clearly wrong problems are due to preprocessor things… with the exception of indenting of comments before a preprocessor directive, I expect the other issues would be relatively rare in most of Chrome.

On Wed, Feb 8, 2017 at 6:26 PM Mark Mentovai <ma...@chromium.org> wrote:

I don’t think that clang-format is ready for us to enforce in this way. I’m impressed with the progress that it’s made, and it’s much better than the last time I looked at it, but I still find myself fighting it fairly regularly. I don’t doubt that if they keep up with improvements, it’ll reach the point where we can start enforcing it more widely.

I ran clang-format in Crashpad and found a number of things that it did that are no better and no worse than what was there before. It did a few things that were clearly better too, but I also found it doing some things that were obviously wrong. Some patterns of bad behavior recur frequently.

I put these notes together with an eye toward providing constructive feedback on why I don’t think we can do this now. I’ve sorted these in mostly descending order of severity. Some of these problems may be specific to the local styles that we use in Crashpad. Running it over any other arbitrary directory may not highlight these quirks, and may reveal others.

Comment formatting before preprocessor #directives is wrong. 1, 2, 3, 4, 5, 6, 7, 8, 9, 10

https://bugs.chromium.org/p/chromium/issues/detail?id=369454 is the bug that was filed for this. The heuristic clang-format uses is if there's a newline between the comment block and the preprocessor directive, the comment will remain indented; otherwise it gets aligned with the preprocessor directive. Personally, I'd find it to be odd to use a newline to get these semantics, so I'd encourage people to star/comment on the bug.

 

In-line comments that overflow onto a new line are reformatted badly. 1, 2

I'm not sure what's going on here. I pasted the snippet into a test.cc file and tried formatting, and the indentation on the comments was maintained. In fact, I'm pretty sure clang-format has some heuristic for maintaining end-of-line comments like this: after the Blink reformat, I saw a lot of end-of-line comments awkwardly squeezed into 20 columns over 4 lines. So… no clue, other than it works correctly for me =/

 

It doesn’t seem to understand all preprocessor directives, like #__has_include_next, and can break their contents. 1, 2

Filed https://llvm.org/bugs/show_bug.cgi?id=31908 upstream for this. Are there other problematic directives?

 

As is commonly mentioned, it’s no good with data tables, which would need to be encased in // clang-format off/on. 1, 2, and a particularly egregious example where it mixes in extra spaces for no good reason, 3

Similar to data tables, sometimes humans insert line breaks into expressions for semantic purposes that clang-format is oblivious to, and these would need //clang-format off/on. It would be nice if it made a stronger effort to not modify not-incorrect code in such cases. 1, 2, 3, 4, 5, 6

This has been something requested in the past (if the formatting isn't wrong, can clang-format just leave it alone), and the answer is no. For example, if identifiers get renamed, etc, it's not easy for clang-format to tell if things need to be reflowed or not (even if no style rules are explicitly forbidden, the resulting code might look quite ugly). Though the style guide does allow flexibility in formatting, clang-format ultimately chooses to just enforce one for consistency.

 

Similarly, it’s not great with long string literals, which would also need // clang-format off/on. 1, 2, 3

If the string literals were already indented, clang-format wouldn't try to reflow them. It's only reflowing them here because it needs to indent, which pushes some of them over the column limit. This does mean it's more annoying to have help text that takes up the full 80 columns…

 

This thing that it does with data tables just means that whenever someone comes along and wants to insert a new element in the middle, or change an existing element so that the column widths need to change, the whole table has to move around, along with blame. 1, 2, 3

It’s also quirky inside macro definitions. It doesn’t seem to understand that a UTF-8 sequence should count as one column (here) and its lack of context makes it do strange

Filed a bug for the UTF-8 formatting, though I suspect that fixing this generally will be non-trivial: https://llvm.org/bugs/show_bug.cgi?id=31909

Also filed a bug for comment formatting in macros that I found when filing that bug: https://llvm.org/bugs/show_bug.cgi?id=31910

 

things gratuitously (here). It seems to enforce different style rules in macros than it does elsewhere (here and several other examples in that file, related to the bit-wise & operator, but oddly not on line 218).

Other interactions with preprocessor #directives seem wrong. Here, it formats the preprocessor-free arrays at lines 309 and 316 with a +4 indent for elements. The array at line 325, which has some preprocessor stuff going on, is inconsistent in that its elements are given a +2 indent.

Sometimes, I can’t tell why it’s doing what it’s doing. It seems to favor reducing the number of lines that code takes in many cases, so it’s puzzling here when it lumps as much as it can onto line 83 and then leaves lines 84 and 85 virtually empty by comparison (similar). It’s also puzzling in these cases (1, 2, 3, 4, 5, 6, 7, 8, 9, 10) where it takes two style-legal lines and turns them into three (here’s a slightly different pattern: 1, 2, 3, 4, 5, 6).

I /think/ things like (1, 2, 3, 4, 5, 6, 7, 8, 9, 10) are from setting AllowAllParametersOfDeclarationOnNextLine to false: it can't fit all the parameters on one line with the function name (even though it could fit on one separate line from the function name), so it formats them one per line instead.

I /think/ things like ( 1, 2, 3, 4, 5, 6) are clang-format trying to group things split across multiple lines by the depth of nesting, i.e. formatting it as if it were a builder operation.

Daniel

[Mark Mentovai]

Daniel Cheng wrote:

Comments inline. I will say that it seems like a lot of the clearly wrong problems are due to preprocessor things… with the exception of indenting of comments before a preprocessor directive, I expect the other issues would be relatively rare in most of Chrome.

Thanks for the response and the bugs filed, Daniel.

On Wed, Feb 8, 2017 at 6:26 PM Mark Mentovai <ma...@chromium.org> wrote:

I don’t think that clang-format is ready for us to enforce in this way. I’m impressed with the progress that it’s made, and it’s much better than the last time I looked at it, but I still find myself fighting it fairly regularly. I don’t doubt that if they keep up with improvements, it’ll reach the point where we can start enforcing it more widely.

I ran clang-format in Crashpad and found a number of things that it did that are no better and no worse than what was there before. It did a few things that were clearly better too, but I also found it doing some things that were obviously wrong. Some patterns of bad behavior recur frequently.

I put these notes together with an eye toward providing constructive feedback on why I don’t think we can do this now. I’ve sorted these in mostly descending order of severity. Some of these problems may be specific to the local styles that we use in Crashpad. Running it over any other arbitrary directory may not highlight these quirks, and may reveal others.

Comment formatting before preprocessor #directives is wrong. 1, 2, 3, 4, 5, 6, 7, 8, 9, 10

https://bugs.chromium.org/p/chromium/issues/detail?id=369454 is the bug that was filed for this. The heuristic clang-format uses is if there's a newline between the comment block and the preprocessor directive, the comment will remain indented; otherwise it gets aligned with the preprocessor directive. Personally, I'd find it to be odd to use a newline to get these semantics, so I'd encourage people to star/comment on the bug.

5, 7, and 9 do have newlines between the comment and the preprocessor directive, but clang-format is still sucking the comments back to the first column.

In-line comments that overflow onto a new line are reformatted badly. 1, 2

I'm not sure what's going on here. I pasted the snippet into a test.cc file and tried formatting, and the indentation on the comments was maintained. In fact, I'm pretty sure clang-format has some heuristic for maintaining end-of-line comments like this: after the Blink reformat, I saw a lot of end-of-line comments awkwardly squeezed into 20 columns over 4 lines. So… no clue, other than it works correctly for me 

Looks like this may have been caused by a local AlignTrailingComments = false. I’ll get rid of that and see what happens (although it may have been there to overcome something else).

[Daniel Cheng]

On Wed, Feb 8, 2017 at 8:16 PM Mark Mentovai <ma...@chromium.org> wrote:

Daniel Cheng wrote:

Comments inline. I will say that it seems like a lot of the clearly wrong problems are due to preprocessor things… with the exception of indenting of comments before a preprocessor directive, I expect the other issues would be relatively rare in most of Chrome.

Thanks for the response and the bugs filed, Daniel.

On Wed, Feb 8, 2017 at 6:26 PM Mark Mentovai <ma...@chromium.org> wrote:

I don’t think that clang-format is ready for us to enforce in this way. I’m impressed with the progress that it’s made, and it’s much better than the last time I looked at it, but I still find myself fighting it fairly regularly. I don’t doubt that if they keep up with improvements, it’ll reach the point where we can start enforcing it more widely.

I ran clang-format in Crashpad and found a number of things that it did that are no better and no worse than what was there before. It did a few things that were clearly better too, but I also found it doing some things that were obviously wrong. Some patterns of bad behavior recur frequently.

I put these notes together with an eye toward providing constructive feedback on why I don’t think we can do this now. I’ve sorted these in mostly descending order of severity. Some of these problems may be specific to the local styles that we use in Crashpad. Running it over any other arbitrary directory may not highlight these quirks, and may reveal others.

Comment formatting before preprocessor #directives is wrong. 1, 2, 3, 4, 5, 6, 7, 8, 9, 10

https://bugs.chromium.org/p/chromium/issues/detail?id=369454 is the bug that was filed for this. The heuristic clang-format uses is if there's a newline between the comment block and the preprocessor directive, the comment will remain indented; otherwise it gets aligned with the preprocessor directive. Personally, I'd find it to be odd to use a newline to get these semantics, so I'd encourage people to star/comment on the bug.

Sigh. Filed https://llvm.org/bugs/show_bug.cgi?id=31911 =)

Daniel

[Ken Rockot]

+1 to the wider presubmit in general but I have one huge pet peeve which I hope can be addressed (I think Mark already pointed this out, but I just hit it again today and it irked me): clang-format loves to reformat code even when it is currently not violating any style rules, e.g. it really wants to change:

  do_something_just_shy_of_80_chars(

      bar, baz, qux);

to

  do_something_just_shy_of_80_chars(bar, baz,

                                                         qux);

I think this is a matter of personal style (right?) and I personally prefer the first one. If I try to format my code that way, clang-format rewrites it anyway. Sad!

Can it be told to not change code that doesn't need to be changed?

[Daniel Cheng]

On Wed, Feb 8, 2017 at 11:04 PM Ken Rockot <roc...@chromium.org> wrote:

+1 to the wider presubmit in general but I have one huge pet peeve which I hope can be addressed (I think Mark already pointed this out, but I just hit it again today and it irked me): clang-format loves to reformat code even when it is currently not violating any style rules, e.g. it really wants to change:

  do_something_just_shy_of_80_chars(

      bar, baz, qux);

to

  do_something_just_shy_of_80_chars(bar, baz,

                                                         qux);

I think this is a matter of personal style (right?) and I personally prefer the first one. If I try to format my code that way, clang-format rewrites it anyway. Sad!

Can it be told to not change code that doesn't need to be changed?

That's not clang-format's design philosophy. See https://goto.google.com/but-my-perfectly-valid-code for the details (sorry, Googlers only for now, but I've asked if this text can be upstreamed into clang-format's documentation).

[Chris Pickel]

On Thu, Feb 9, 2017 at 3:25 AM, Mark Mentovai <ma...@chromium.org> wrote:

Similar to data tables, sometimes humans insert line breaks into expressions for semantic purposes that clang-format is oblivious to, and these would need //clang-format off/on. It would be nice if it made a stronger effort to not modify not-incorrect code in such cases. 1, 2, 3, 4, 5, 6

It's not always necessary to disable clang-format to preserve line-breaks. A trailing comment on a line will be kept with that line, so in 1, you can write it like this:

  ASSERT_EQ(sizeof(MinidumpModuleCrashpadInfoList) +                  //

                sizeof(MinidumpModuleCrashpadInfoLink) +              //

                sizeof(MinidumpModuleCrashpadInfo) +                  //

                sizeof(MinidumpRVAList) +                             //

                sizeof(RVA) +                                         //

                sizeof(MinidumpSimpleStringDictionary) +              //

                sizeof(MinidumpSimpleStringDictionaryEntry) +         //

                sizeof(MinidumpUTF8String) + arraysize(kEntry) + 2 +  // padding

                sizeof(MinidumpUTF8String) + arraysize(kKey) +        //

                sizeof(MinidumpUTF8String) + arraysize(kValue),       //

and clang-format won't touch it. I won't argue it's pretty, but I'd argue it's better to help clang-format do what you want than to turn it off.

I'm fully supportive of clang-format *being* on, but I don't think it's a good idea to turn it on in a repository that isn't yet clang-formatted.

[Alex Clarke]

+1

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---

You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Marijn Kruisselbrink]

I'mm all for turning on clang format for as much code as possible. But shouldn't you have waited more than ~6 hours before landing this after starting the discussion? Not much point in starting a discussion if you're not going to wait for people to give their opinion.

[Peter Kasting]

On Thu, Feb 9, 2017 at 9:43 AM, Marijn Kruisselbrink <m...@chromium.org> wrote:

I'mm all for turning on clang format for as much code as possible. But shouldn't you have waited more than ~6 hours before landing this after starting the discussion? Not much point in starting a discussion if you're not going to wait for people to give their opinion.

Especially when much of Chrome MTV + SFO was on a company offsite Feb. 8-9?  Please be kinder with the discussion window on a change that affects the whole codebase like this.  One day would never have been enough time to agree on this, and it is especially not enough right now :).

FWIW, I am with Mark on this one (less negative than I used to be, don't think this is ready to turn on globally), though I am not prepared to stand in the way of doing it if people don't judge Mark's cases and the other miscellaneous ones that still occur to be problematic.  It is telling to me, though, that the people who have long expressed the most reservations about this are all people who have served as readability reviewers.

Perhaps not a blocker for this move, but clang-format recently has been modifying the wrapping and indenting of code in ways that seem nondeterministic.  For example, when writing https://codereview.chromium.org/2663013003/ , I was at different times given this wrapping:

  column_set->AddPaddingColumn(

      0,

      delegate->GetMetric(

          LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING));

and at times:

  column_set->AddPaddingColumn(

      0, delegate->GetMetric(

            LayoutDelegate::Metric::RELATED_CONTROL_HORIZONTAL_SPACING));

...even though in both cases, I was using identifiers that would fit with either wrapping.  I could not figure out what triggered one versus the other (I would have assumed I'd get the second style unless it wouldn't fit, and otherwise I'd get the first style, but as you can see from the uploaded patch that's not the case; yet the first style isn't globally preferred either, since I've gotten the second style on subsequent patches I've written).

I must be missing some piece of evidence in trying to understand this.

PK

[Kevin Bailey]

If the tool is going to be more tightly integrated into the source control, it would be nice if it at least remembered our preferences. If you ran the tool, and changed some lines back, it would be handy if it remembered that and didn't touch them again (until you changed them, or the preceding indent.)

--

[dan...@chromium.org]

On Fri, Feb 10, 2017 at 10:07 AM, Kevin Bailey <k...@chromium.org> wrote:

If the tool is going to be more tightly integrated into the source control, it would be nice if it at least remembered our preferences. If you ran the tool, and changed some lines back, it would be handy if it remembered that and didn't touch them again (until you changed them, or the preceding indent.)

The presubmit is based on "git cl format" which only formats around lines you're changing in your CL. It doesn't reformat the whole file you touch.

[John Abd-El-Malek]

(sorry, missed the replies yesterday)

Mark: without going into details of the issues you point out, the reasoning was that we're already dealing with all these formatting issues through a) people running git cl format ., b) repo-wide changes running git cl format and c) a large part of the code was already using this. Since this is only an upload presubmit check, it can be ignored if you choose (and it doesn't affect other repos like crashpad). 

Marijn: I assumed waiting MTV working hours was sufficient, although in hindsight I should have waited 24 hours to give other timezones time to voice opposition (although luckily in this case, that didn't seem to come up). Since the overwhelming feedback (over the thread, and on IMs or people talking about it in person) was positive, it felt like there was consensus.

Peter: sorry, I didn't think of the trip. That was a mistake.

In the end, a change like this is never going to get 100% agreement one way or another. But it should have a big majority backing it IMO. Does anyone disagree with the previous two statements?

If others disagree that this change should be made, please speak up (whether on this thread or on IM/private email). If it's more than a few people (i.e. plausbily it's not a majority that want this change), I'm happy to create a survey and send it out to gauge opinon and then we can roll it back if necessary.

[Dan Beam]

On Fri, Feb 10, 2017 at 10:32 AM, John Abd-El-Malek <j...@chromium.org> wrote:

(sorry, missed the replies yesterday)

Mark: without going into details of the issues you point out, the reasoning was that we're already dealing with all these formatting issues through a) people running git cl format ., b) repo-wide changes running git cl format and c) a large part of the code was already using this. Since this is only an upload presubmit check, it can be ignored if you choose (and it doesn't affect other repos like crashpad). 

Hey John,

In looking at your patch, I see a CheckPatchFormatted() in CheckChangeOnCommit() as well:

https://codereview.chromium.org/2680413002/diff/1/PRESUBMIT.py

This leads me to believe it'd stop a CQ.  Is there something I'm missing here?

When you say "Since this is only an upload presubmit check, it can be ignored if you choose", does this mean you're encouraging NOPRESUBMIT=true in CL descriptions to get around clang-format?

-- Dan

[Daniel Cheng]

On Fri, Feb 10, 2017 at 11:13 AM Dan Beam <db...@chromium.org> wrote:

On Fri, Feb 10, 2017 at 10:32 AM, John Abd-El-Malek <j...@chromium.org> wrote:

(sorry, missed the replies yesterday)

Mark: without going into details of the issues you point out, the reasoning was that we're already dealing with all these formatting issues through a) people running git cl format ., b) repo-wide changes running git cl format and c) a large part of the code was already using this. Since this is only an upload presubmit check, it can be ignored if you choose (and it doesn't affect other repos like crashpad). 

Hey John,

In looking at your patch, I see a CheckPatchFormatted() in CheckChangeOnCommit() as well:

https://codereview.chromium.org/2680413002/diff/1/PRESUBMIT.py

This leads me to believe it'd stop a CQ.  Is there something I'm missing here?

When you say "Since this is only an upload presubmit check, it can be ignored if you choose", does this mean you're encouraging NOPRESUBMIT=true in CL descriptions to get around clang-format?

The canned check only emits a warning, so it doesn't block the CQ:

https://cs.chromium.org/chromium/tools/depot_tools/presubmit_canned_checks.py?rcl=b1550ec8b36f49eb67f41ed8e4582aedd1e0ade7&l=1140

(I was wondering this several weeks ago when I noticed unformatted Blink patches landing)

Daniel

[Torne (Richard Coles)]

Presubmit warnings always seem to block the commit queue in my experience.

[Nico Weber]

I just gave it a try and this doesn't stop presubmit from becoming green: https://codereview.chromium.org/2690603002/ => https://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/361842/steps/presubmit/logs/stdio (warning about things not being formatted, but goes through anyhow).

[Theresa Wellington]

Now that this is a presumit warning, several discrepancies between git cl format and other Java presubmit checks have been identified (e.g. crbug.com/656821 and crbug.com/728623). What are the correct labels or folks to cc on crbugs to get these resolved?

Thank you,

Theresa

[Dirk Pranke]

Good question. This particular functionality isn't well-owned at the moment, I'd guess. I can take a look at it but it may not get an immediate diagnosis and fix :).

-- Dirk

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/d4479d48-0349-451d-9d24-2f59358974bc%40chromium.org.

[Daniel Cheng]

Do we consider the bug to be on clang-format or the Java presubmit checks? If it's the latter, maybe we can just delete those presubmit checks? *shrug*

Daniel

[Nico Weber]

https://chromium.googlesource.com/chromium/src/+/lkcr/docs/clang_format.md#Reporting-problems mentions how to report clang-format bugs.

As Daniel says, the bug here is that we have two systems competing to check Java formatting. We should turn one off.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAF3XrKrDvvu-qPxSMvPoxJqdpUysCJ2MoF-WdeBrRSgtu5NTqg%40mail.gmail.com.

[Tommy Nyquist]

I think we've had the presubmit tool for Java style for a while?

Could we then wait to turn on clang-format for Java until they match better?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGbLiHXgTyLmQXd6fm4PD4S_LoF3vf%3DaYz_eN-4jKJenY%3DGXQ%40mail.gmail.com.

[Nico Weber]

Having both on is silly, so I don't know if there's much point in making them match.

If y'all don't consider clang-format good enough to be used for java code and prefer manually formatting your java code to match the presubmit thingy, we should disable clang-format. (And fix the issues that are considered blockers -- let me know which bugs these are.)

Else we should disable the presubmit tool.

So the bar shouldn't be "do they match" but "is clang-format good enough".

[Nico Weber]

On Thu, Jun 15, 2017 at 8:51 PM, Nico Weber <tha...@chromium.org> wrote:

Having both on is silly, so I don't know if there's much point in making them match.

If y'all don't consider clang-format good enough to be used for java code and prefer manually formatting your java code to match the presubmit thingy, we should disable clang-format. (And fix the issues that are considered blockers -- let me know which bugs these are.)

Else we should disable the presubmit tool.

So the bar shouldn't be "do they match" but "is clang-format good enough".

^ I filed https://bugs.chromium.org/p/chromium/issues/detail?id=768576 for this.