clang-format vs. style guide

[Steven Bennetts]

The Chromium style guide says:

[Scott Graham]

I like this new style guide. Very Zen.

On Thu, Aug 22, 2013 at 11:14 AM, Steven Bennetts <stev...@google.com> wrote:

The Chromium style guide says:

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

[Steven Bennetts]

Right. So. GMail + user interaction fail. Without getting into the demerits of ctrl-enter being 'Send' or the shortcomings of certain keyboards...

The Google Style Guide says:

"Constructor initializer lists can be all on one line or with subsequent lines indented four spaces."

// When it requires multiple lines, indent 4 spaces, putting the colon on
// the first initializer line:
MyClass::MyClass(int var)
    : some_var_(var),             // 4 space indent
      some_other_var_(var + 1) {  // lined up
  ...
  DoSomething();
  ...
}

Which clang-format turns into (with a longer class name and comments removed):

MyClassWithALongName::MyClassWithALongName(int var)

    : some_var_(var), some_other_var_(var + 1) {

  DoSomething();

}

Should we be accepting the clang format? It isn't entirely clear to me that the clang formatting is forbidden by the style guide, but I have had push back in the past against multiple initializers on one line (when not on the same line as the constructor declaration).

[Rouslan Solomakhin]

I'd say it's a matter of preference of the code owner. Also be consistent with the surrounding code.

[Scott Graham]

(In an attempt to be less "funny")

I had the same question and filed this internal bug against clang-format: http://b/9620984

The summary is that the style arbiters said one line is fine, though that's Google of course, not Chromium. I feel like Chromium doesn't have any reason to diverge from Google style here.

[Peter Kasting]

On Thu, Aug 22, 2013 at 11:33 AM, Scott Graham <sco...@chromium.org> wrote:

I had the same question and filed this internal bug against clang-format: http://b/9620984

The summary is that the style arbiters said one line is fine, though that's Google of course, not Chromium. I feel like Chromium doesn't have any reason to diverge from Google style here.

I would say the clang-format style is legal; the one-arg-per-line style is much more common in Chromium code, however (and seems fractionally more readable to me), so unless there's surrounding code that already does multiple args per line, I'd normally prefer to see one arg per line.  And would probably say so in the review comments.

This is something like the fourth case I've seen recently where clang-format does something in the range of "less common in Chrome code" to "arguably illegal by the Chrome style guide".  I have to say that the more of these I run into, the less favorable I feel towards people using clang-format to format code.

PK

[Mark Mentovai]

Steven Bennetts wrote:

MyClassWithALongName::MyClassWithALongName(int var)

    : some_var_(var), some_other_var_(var + 1) {

  DoSomething();

}

I think this formatting is fine in Chromium. It occurs infrequently, but it’s not outlawed.

[Rachel Blum]

Well, as long as we keep our style guide ambiguous, a tool like clang-format is not that helpful. My personal preference would be an unambiguous style guide, but if we don't want that, we should probably not allow automated reformatting around rules that allow choice.

After all, I run clang-format to get less style nits, not different style nits :)

- rachel

--

[Jeffrey Yasskin]

On Thu, Aug 22, 2013 at 12:09 PM, Peter Kasting <pkas...@chromium.org> wrote:

> This is something like the fourth case I've seen recently where clang-format
> does something in the range of "less common in Chrome code" to "arguably
> illegal by the Chrome style guide". I have to say that the more of these I
> run into, the less favorable I feel towards people using clang-format to
> format code.

When you run into things like this, please file bugs against
clang-format. While its authors strongly prefer to keep the Google
style and the Chromium style the same (because the Chromium style
guide says that's our goal too), if you and the other leaders of the
Chromium project agree on something different, they'll do it.

[Peter Kasting]

On Thu, Aug 22, 2013 at 12:22 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

On Thu, Aug 22, 2013 at 12:09 PM, Peter Kasting <pkas...@chromium.org> wrote:

> This is something like the fourth case I've seen recently where clang-format
> does something in the range of "less common in Chrome code" to "arguably
> illegal by the Chrome style guide".  I have to say that the more of these I
> run into, the less favorable I feel towards people using clang-format to
> format code.

When you run into things like this, please file bugs against
clang-format.

When I have asked for clang-format to change, I've gotten strong pushback that unless something is outright illegal, they are unwilling to change for mere "common practice".  If it's legal in Chromium, and it's what the Google formatter does, then why change it?  And I have to admit that this is difficult to argue against when our stated goal is to generally be compatible with Google style.

PK

[Stuart Morgan]

On Thu, Aug 22, 2013 at 12:16 PM, Rachel Blum <gr...@chromium.org> wrote:

Well, as long as we keep our style guide ambiguous, a tool like clang-format is not that helpful.

It depends on how we choose to handle ambiguity as team. One option is "the style guide allows several options, so as the owner of this section of code so I'll pick the one I like and force all CLs in this code to follow that style". Another is "the style guide allows several options, and this is one of them, so I won't say anything".

If we do more of the latter, then clang-format is perfectly compatible with an ambiguous guide. And while I know that it's not always the case that all options are equally readable (as discussed at length in the recent thread about mandatory style-tool runs), I do think we do too much of the former, wasting time and annoying people. When we decide as a team not to have a rule about something in order to reduce style guide complexity, but what happens in practice is that everyone has to try to memorize a different per-person style guide for everyone they send reviews to, with conflicting rules, we have failed at reducing complexity.

(As an example, we used to have a rule about whether {} were allowed for one-line ifs, and we explicitly decided to get rid of that rule. I've been pushing back hard against anyone who tries to impose their personal preference on that in reviews, because it's a time sink to have every CL be a coin flip about whether the perfectly style-conformant 'if' that person A writes is going to match the preference of person B. If we decided that it wasn't important enough to have a style rule about, then it shouldn't be important enough to nit on CLs about.)

-Stuart

[Peter Kasting]

On Thu, Aug 22, 2013 at 12:33 PM, Stuart Morgan <stuart...@chromium.org> wrote:

If we decided that it wasn't important enough to have a style rule about, then it shouldn't be important enough to nit on CLs about.

I explicitly disagree with this.  If a particular file, directory, or subsystem is consistent about style X, then in general new code in that scope should follow style X, and if it doesn't, the reviewer _should_ complain about it.  Exceptions are possible but should have justification.  Similarly, though -- and I admit that this one is hard for me -- if a subsystem is consistent about style X, but a reviewer likes style Y better, the reviewer should not ask for style Y for code in that subsystem; indeed, the reviewer should actively enforce style X.

The style guide is a tool to enforce the ultimate goal of consistency, as opposed to consistency being a goal insofar as it is necessary to fulfill the letter of the style guide.

PK

[Jeffrey Yasskin]

Yeah, I think either we need to decide that clang-format's Google-ish
output is acceptable even over personal preferences from the code
owner, or we need to explicitly abandon the goal of exact
compatibility with Google style and get clang-format to match Chromium
practices better.

I really don't care which, but you and the other Chromium style folks
should make a decision. :)

[Jeffrey Yasskin]

On Thu, Aug 22, 2013 at 1:17 PM, Daniel Jasper <dja...@google.com> wrote:
> So from the side of clang-format's developers:
>
> There are several reasons why we push back on possibly unnecessary
> deviations between Google style and Chromium style:
> - It feels like we are fighting battles we have fought before. Almost every
> single style decision is debatable and debated. We are very happy if we can
> finally come to a conclusion that it is in line with the style guide and
> seems to make most people happy. Then starting over the exact same
> discussion for a different style guide is exhausting. I am aware that this
> is not a good reason to do something, I just want to shed a bit of light on
> why we might sometimes push back stronger than appropriate.
> - There (at least currently) is the stated goal for Google style and
> Chromium style not to deviate unnecessarily. And there are several
> advantages to that.
> - We are limited on resources. We still have quite an extensive bug-list as
> well as a few features we want to implement. It is hard for us to prioritize
> changing a specific style issue in Chromium style without even knowing how
> the preferences are in Chromium or who would be responsible to make
> decisions. Now, we have developed clang-format completely as and open-source
> project and contributions are always welcome. So, if you decide to go down
> the road of deviating more from Google style, helping us would certainly
> speed up the corresponding changes to clang-format.
>
> Lastly, while we know, it would be awesome to have a tool that can be taken
> as "the law" and would always do the right thing (for everybody), we are not
> sure whether that is even feasible. However, we still hope that clang-format
> gives a lot of benefit if used during code development. In our experience,
> it is good enough most of the time (i.e. >90% of the time) and if there is
> something that does not conform to the local style or that you can format
> better, then fixing that up "by hand" is usually quite fast. Even looking at
> one of the discussed issues (conditional operator placement): There are
> roughly 1.5k instances in the Chromium codebase, i.e. a lot less then 1%. So
> you should not be running into this frequently.
>
> Cheers,
> Daniel

Thanks for the reply.

Regarding the resources and battles questions, would it help to check
a .clang-format file (http://clang.llvm.org/docs/ClangFormat.html)
into the Chromium root directory instead of baking the decisions into
clang-format itself? Then changes in how to set existing options could
go through Chromium's normal review process instead of needing you
folks to do something.

Jeffrey

[Stuart Morgan]

On Thu, Aug 22, 2013 at 12:38 PM, Peter Kasting <pkas...@chromium.org> wrote:

I explicitly disagree with this.  If a particular file, directory, or subsystem is consistent about style X, then in general new code in that scope should follow style X, and if it doesn't, the reviewer _should_ complain about it.

Isn't this arguing for fiefdoms of enforced, conflicting styles within Chromium? I would think we would want to discourage that. Chromium code is Chromium code. If the team as a whole thinks two styles for some minor thing are essentially equally readable, what's the benefit to the team in creating enforced pockets of differences, just because a local owner likes that style better?

If we are saying as a team that everyone can easily read (again, just picking an example; substitute your favorite style nit as applicable):

  if (foo)

    bar;

and

  if (foo) {

    bar;

  }

then what does it matter if I the two places someone sees it are different functions in a file, different files in the same directory, or different directories? When you are reading code, do you actual stop every time you change files, reset your mental model of what code should look like based on parsing the path against a list of mental rules, and then get thrown off if some nit is different?

Obviously there are extreme cases (e.g, four one-line if statements all in a row should clearly use the same style), but beyond that, does it really matter? Do we actually want to encourage a world where when someone working on Chromium works on a different part of the code than they usually do, they have to play by different rules, code looks different than they are used to, and reviews are more time consuming for everyone because of a bunch of nits about things that would be perfectly fine one directory over?

And if I end inheriting ownership of two different sections of code, each with, say, different rules about whether constructor arguments should be on one line if possible, or one line per argument, do I now have a responsibility to keep a mapping about which is the "right" style for each component, and refer to it every time I have a code review—multiplied by the number of nits that different between those two directories, multiplied again by the actual number of directories I own? Or now that I'm the owner, does my preference dominate for all new code (meaning it was never actually about local consistency at all, but personal preference, because now parts of the code that were internally consistent on nits will become less so)?

I would much rather live in a world where two files next to each other have different constructor lists, and two functions in a file have different one-line if bracing, than live in that world.

The style guide is a tool to enforce the ultimate goal of consistency, as opposed to consistency being a goal insofar as it is necessary to fulfill the letter of the style guide.

Completely agreed. But I think taking that to the level of carving up Chromium into a bunch of deliberately inconsistent islands based on the whims of OWNERS files is bad for consistency, not good.

-Stuart

[Peter Kasting]

On Thu, Aug 22, 2013 at 2:29 PM, Stuart Morgan <stuart...@chromium.org> wrote:

On Thu, Aug 22, 2013 at 12:38 PM, Peter Kasting <pkas...@chromium.org> wrote:

I explicitly disagree with this.  If a particular file, directory, or subsystem is consistent about style X, then in general new code in that scope should follow style X, and if it doesn't, the reviewer _should_ complain about it.

Isn't this arguing for fiefdoms of enforced, conflicting styles within Chromium? I would think we would want to discourage that. Chromium code is Chromium code. If the team as a whole thinks two styles for some minor thing are essentially equally readable, what's the benefit to the team in creating enforced pockets of differences, just because a local owner likes that style better?

These are precisely the same arguments that have come up in the past when people suggested mandating clang-format (which I don't like without an exhaustive style guide) as well as when people have suggested an exhaustive style guide (which I do like and which would make me happy about clang-format as well).  Both have been rejected.  Given that, I find myself in a world where the style guide explicitly allows multiple styles but also mandates as the single most important rule that people "be consistent with the surrounding code".  And yes, the following things are true for me:

* I notice things like braces on conditionals and get thrown off if something is different than the surrounding code.  I really am that detail-driven.  This kind of thing drives me nuts, constantly.  It would probably be nice if it didn't, but it does.

* When I have sole ownership of code sections with different styles, I attempt to enforce the existing style for those two sections where reasonable (especially for small changes), but at the same time try and make more sweeping changes to bring the overall styles of the sections into congruence, in order to not remain in a world of such disparities forever.

* I keep track mentally of which reviewers like which styles so I don't intentionally send some OWNER code in a format they dislike.

I am not perfect and certainly have violated the "don't promote personal preference over consistency" rule before, but I do my best.  I continually wish that we could avoid all this by just making the style guide give an explicit ruling on every issue (and then having auto-formatters handle and enforce it), but I tried my best to make Chrome be that world and got slapped down very hard for doing so.

PK

[Thiago Farina]

Can we get that added? I'd be very happy with it getting checked in,
since I having been using clang-format (from inside vim mostly) to
format code I write.

Chromium style for me was always more about C++ constructions rather
than style (how you format code), about style I have always followed
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml. I
don't think there is a Chromium equivalent to that doc.

Chromium coding style has only a single section about coding format:
http://www.chromium.org/developers/coding-style

So I don't know about what clang-format chromium style we are talking
about. May be the style that is present in the codebase that diverges
from what is recommended by the Google C++ Style guide? If it's that I
found this very questionable.

--
Thiago

[Stuart Morgan]

On Thu, Aug 22, 2013 at 2:49 PM, Peter Kasting <pkas...@chromium.org> wrote:

but also mandates as the single most important rule that people "be consistent with the surrounding code".

FWIW, the rule is actually "Use common sense and BE CONSISTENT"; I think the common sense aspect is something that often gets lost in zeal to enforce a particular set of preferences that the guide is silent on. That section goes on to say "But local style is also important. If code you add to a file looks drastically different from the existing code around it, the discontinuity throws readers out of their rhythm when they go to read it. Try to avoid this." (emphasis mine)

I'm of the opinion that the kind of style-guide-ambiguous nits I see a lot of wrangling over in reviews generally do not fall into the bucket of "drastically different from the existing code", and also (for the reasons I've mentioned) that 'surrounding code' should generally be interpreted to mean "Chromium" not "a specific OWNER's sphere of influence".

Obviously there are different ways of interpreting that part of the guide, and we aren't all going to agree. But I would urge people who find themselves regularly flagging nits that are for things that are not in the guide to stop and ask themselves whether there's a good argument that the change being requested is objectively good for the overall readability of the Chromium codebase—and thus a good use of their time and the reviewee's time—or is just enforcing a personal preference in a known-contentious case.

-Stuart

[Daniel Jasper]

No, that would not help much. Changing those flags is easy (in fact, you can just submit them to the open-source project if you want them changed). However, none of the things in question are current flags or can be made flags without significant effort (both initial effort and maintenance effort). The maintenance effort stems from ensuring that all combinations of flags still do reasonable things...

Cheers,
Daniel

Jeffrey

[Jeffrey Yasskin]

And for extra context, the list of easy-to-change flags is at
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Format/Format.h?view=markup.

[John Abd-El-Malek]

Apologies in advance for digging up an old thread, and a formatting one at that. But.. from a code review I was doing, clang-format was done and it did two things:

in x.cc, it changed

void Foo::Bar() {

  DoY();

}

to

void Foo::Bar() { DoY(); }

which I haven't seen in our code before or in the style guide.

Also, in a header it changed

void LongFunctionName(

  type parameter1,

  type parameter2) OVERRIDE;

to

void LongFunctionName(type parameter1,

                                    type parameter2)

    OVERRIDE;

The latter form of the override is odd to read, and something we avoided before. I got pointed to b/11514126 and a doc about style decisions for clang-format (sorry, google.com only). In it, the final decision was to change override/const to not start on their own line. However, the clang-format team apparently ignore the OVERRIDE version (i.e. the macro one, which we use in chrome, but isn't used in google3). It would be great for whoever deals with the team gets them to consider OVERRIDE as well.

In the meantime, I cringe about changes like this landing in the code and being used as precedent for other changes.

--

[Peter Kasting]

On Mon, Apr 7, 2014 at 2:43 PM, John Abd-El-Malek <j...@chromium.org> wrote:

In the meantime, I cringe about changes like this landing in the code and being used as precedent for other changes.

+1.  It would be nice to add both these to the list of things clang-format shouldn't do to Chromium code.

PK 

[Daniel Cheng]

I've been doing research in preparation for filing clang-format bugs for these two issues. For the first example you gave where it collapsed a single-line body on to the same line as the { and }, I find at least two style rules that apply:

- The open curly brace is always at the end of the same line as the last parameter.

- The close curly brace is either on the last line by itself or (if other style rules permit) on the same line as the open curly brace.

Doesn't following the second rule by definition violate the first rule? The style guide itself gives an example (that doesn't compile) that does this: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namespaces#Namespaces -- search for AtEof(). Offhand, I don't know if there are other style rules that would prohibit 3 lines from being collapsed into 1, but that doesn't mean they don't exist.

Daniel

--

[Peter Kasting]

On Mon, Apr 7, 2014 at 2:52 PM, Daniel Cheng <dch...@chromium.org> wrote:

I've been doing research in preparation for filing clang-format bugs for these two issues. For the first example you gave where it collapsed a single-line body on to the same line as the { and }, I find at least two style rules that apply:

- The open curly brace is always at the end of the same line as the last parameter.

- The close curly brace is either on the last line by itself or (if other style rules permit) on the same line as the open curly brace.

Doesn't following the second rule by definition violate the first rule?

Yes if you read "at the end of" in the first rule to mean "the last character in".  I believe the wording was actually intended to imply "is after the last parameter, on the same line".

 

The style guide itself gives an example (that doesn't compile) that does this: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namespaces#Namespaces -- search for AtEof(). Offhand, I don't know if there are other style rules that would prohibit 3 lines from being collapsed into 1, but that doesn't mean they don't exist.

Classically in Chromium we've allowed this sort of collapsing if and only if the function is a unix_hacker()-style function (an accessor or other cheap function inlined into the declaration):

foo.h:

class Foo {

 public:

  int bar() const { return bar_; }  // OK because inlining bar() was OK

  int other_bar() const {  // This wrapping is also OK

    return bar_;

  }

  void Baz() { DoStuff(); }  // Not OK because all non-unix_hacker()-functions should have the function body on a new line

foo.cc:

int third_bar() const { return bar_; }  // This should have been inlined into the header

int Foo::Qux() const { return 3; }  // Not OK

There are no written rules governing this convention that I'm aware of.

I would probably summarize this as, clang-format should always put a carriage return after an open brace unless the function name indicates it is a cheap accessor, and it is defined where it is declared.

PK

[Nico Weber]

On Mon, Apr 7, 2014 at 2:43 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Apologies in advance for digging up an old thread, and a formatting one at that. But.. from a code review I was doing, clang-format was done and it did two things:

in x.cc, it changed

void Foo::Bar() {

  DoY();

}

to

void Foo::Bar() { DoY(); }

which I haven't seen in our code before or in the style guide.

Also, in a header it changed

void LongFunctionName(

  type parameter1,

  type parameter2) OVERRIDE;

to

void LongFunctionName(type parameter1,

                                    type parameter2)

    OVERRIDE;

Have you seen this recently? I think this changed in clang-format a while ago, and we updated our clang-format binary to a version with that fix 2-3 weeks ago.

[Jeffrey Yasskin]

On Mon, Apr 7, 2014 at 2:43 PM, John Abd-El-Malek <j...@chromium.org> wrote:

> Apologies in advance for digging up an old thread, and a formatting one at
> that. But.. from a code review I was doing, clang-format was done and it did
> two things:
>
> in x.cc, it changed
> void Foo::Bar() {
> DoY();
> }
>
> to
> void Foo::Bar() { DoY(); }
>
> which I haven't seen in our code before or in the style guide.

IIRC, this one happened when they made single-line functions collapse
to one line in header files. e.g.
https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/browsing_instance.h&sq=package:chromium&q=BrowsingInstance::browser_context&type=cs&l=61

They should split AllowShortFunctionsOnASingleLine to support
different settings between functions inside vs outside class
definitions.

Since you work at Google, can you file a bug at
http://go/clang-format-bug? Non-googlers can probably file bugs
against http://llvm.org/bugs/enter_bug.cgi?product=clang, or wherever
Daniel Jasper says.

> Also, in a header it changed
> void LongFunctionName(
> type parameter1,
> type parameter2) OVERRIDE;
>
> to
>
> void LongFunctionName(type parameter1,
> type parameter2)
> OVERRIDE;
>
>
> The latter form of the override is odd to read, and something we avoided
> before. I got pointed to b/11514126 and a doc about style decisions for
> clang-format (sorry, google.com only). In it, the final decision was to
> change override/const to not start on their own line. However, the
> clang-format team apparently ignore the OVERRIDE version (i.e. the macro
> one, which we use in chrome, but isn't used in google3). It would be great
> for whoever deals with the team gets them to consider OVERRIDE as well.

You can do this by commenting on the bug you pointed out. Since they
accepted the google style arbiters' decision, they ought to accept the
equivalent folks from the chrome team, like you and Peter.

Re Nico: I see this today.

[John Abd-El-Malek]

On Mon, Apr 7, 2014 at 3:10 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

On Mon, Apr 7, 2014 at 2:43 PM, John Abd-El-Malek <j...@chromium.org> wrote:

> Apologies in advance for digging up an old thread, and a formatting one at
> that. But.. from a code review I was doing, clang-format was done and it did
> two things:
>
> in x.cc, it changed
> void Foo::Bar() {
>   DoY();
> }
>
> to
> void Foo::Bar() { DoY(); }
>
> which I haven't seen in our code before or in the style guide.

IIRC, this one happened when they made single-line functions collapse
to one line in header files. e.g.
https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/browsing_instance.h&sq=package:chromium&q=BrowsingInstance::browser_context&type=cs&l=61

You say "header files", but my example was in a .cc file?

Nico: they changed this for "override" but not "OVERRIDE".

I had commented on the doc. Should I also file a bug?

[Daniel Cheng]

I've already filed a bug for OVERRIDE -- see http://llvm.org/bugs/show_bug.cgi?id=19363

Daniel

[Jeffrey Yasskin]

On Mon, Apr 7, 2014 at 3:15 PM, John Abd-El-Malek <j...@chromium.org> wrote:
>
>
>
> On Mon, Apr 7, 2014 at 3:10 PM, Jeffrey Yasskin <jyas...@chromium.org>
> wrote:
>>
>> On Mon, Apr 7, 2014 at 2:43 PM, John Abd-El-Malek <j...@chromium.org>
>> wrote:
>> > Apologies in advance for digging up an old thread, and a formatting one
>> > at
>> > that. But.. from a code review I was doing, clang-format was done and it
>> > did
>> > two things:
>> >
>> > in x.cc, it changed
>> > void Foo::Bar() {
>> > DoY();
>> > }
>> >
>> > to
>> > void Foo::Bar() { DoY(); }
>> >
>> > which I haven't seen in our code before or in the style guide.
>>
>> IIRC, this one happened when they made single-line functions collapse
>> to one line in header files. e.g.
>>
>> https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/browsing_instance.h&sq=package:chromium&q=BrowsingInstance::browser_context&type=cs&l=61
>
>
> You say "header files", but my example was in a .cc file?

Right. As I said in my next paragraph, they didn't make all the
distinctions they should have, probably on the assumption that they
should start with the simplest behavior, and make it more complicated
as the need arose. It's worth filing a bug for this.

[Daniel Jasper]

I probably can't post to chromium-dev, but I assume that enough people will get this email..

About override/OVERRIDE: If you read the comment back-log on the mentioned document you can see that I brought this up, but my approach was overruled. We all overlooked the necessity for this inside Chromium's codebase, though, so I'll see what I can do.

About short functions on a single line: I am not going to implement any further support beyond the flag AllowShortFunctionsOnASingleLine (which you can set to whatever value in Chromium's .clang-format) file. I personally don't think that more complex rules are an improvement or matter in any significant way (readability or consistency), but I appreciated that opinions differ. If you care enough, contribute a patch, clang-format is an open-source project.

Cheers,
Daniel

[Nico Weber]

On Mon, Apr 7, 2014 at 11:10 PM, Daniel Jasper <dja...@google.com> wrote:

I probably can't post to chromium-dev, but I assume that enough people will get this email..

About override/OVERRIDE: If you read the comment back-log on the mentioned document you can see that I brought this up, but my approach was overruled. We all overlooked the necessity for this inside Chromium's codebase, though, so I'll see what I can do.

About short functions on a single line: I am not going to implement any further support beyond the flag AllowShortFunctionsOnASingleLine (which you can set to whatever value in Chromium's .clang-format) file.

We shouldn't set anything in our .clang-format file except style=Chromium. If we feel that any of the defaults for style=Chromium aren't good, we should change what style=Chromium means in clang-format. It'd be silly if chromium used "style=Chromium, but with the following changes" :-)

 

[Thiago Farina]

On Tue, Apr 8, 2014 at 3:10 AM, Daniel Jasper <dja...@google.com> wrote:

I probably can't post to chromium-dev, but I assume that enough people will get this email..

About override/OVERRIDE: If you read the comment back-log on the mentioned document you can see that I brought this up, but my approach was overruled. We all overlooked the necessity for this inside Chromium's codebase, though, so I'll see what I can do.

About short functions on a single line: I am not going to implement any further support beyond the flag AllowShortFunctionsOnASingleLine (which you can set to whatever value in Chromium's .clang-format) file. I personally don't think that more complex rules are an improvement or matter in any significant way (readability or consistency), but I appreciated that opinions differ.

Who is responsible for implementing the Chromium style in clang-format? Manuel, Chandler? If clang-format -style=Chromium does something different than what we expect for Chromium code, than there is something wrong and we should fix it to adhere to the style we prefer and have used over the past of what, 8 years?

I don't think we should let a tool make the readability of our code base worse.

Yes, I found this:

void MyClass::DoSomething() { blablah }

pretty worse.

As we always have formatted it this way:

void MyClass::DoSomething() {

  blablah

}

and imo, clang-format should respect that.

-- 

Thiago Farina

[Nico Weber]

On Tue, Apr 8, 2014 at 8:45 AM, Nico Weber <tha...@chromium.org> wrote:

On Mon, Apr 7, 2014 at 11:10 PM, Daniel Jasper <dja...@google.com> wrote:

I probably can't post to chromium-dev, but I assume that enough people will get this email..

About override/OVERRIDE: If you read the comment back-log on the mentioned document you can see that I brought this up, but my approach was overruled. We all overlooked the necessity for this inside Chromium's codebase, though, so I'll see what I can do.

About short functions on a single line: I am not going to implement any further support beyond the flag AllowShortFunctionsOnASingleLine (which you can set to whatever value in Chromium's .clang-format) file.

We shouldn't set anything in our .clang-format file except style=Chromium. If we feel that any of the defaults for style=Chromium aren't good, we should change what style=Chromium means in clang-format. It'd be silly if chromium used "style=Chromium, but with the following changes" :-)

…Daniel did just that here: http://llvm.org/viewvc/llvm-project?view=revision&revision=205760 So non-inline methods should no longer be on one line once our copy of clang-format is updated to include that.

[Daniel Jasper]

Both issues are now fixed at head.

To be more precise, I mean you can temporarily add flags to the .clang-format file so that there is an established decision mechanism as well as fast turn-around. I'll then port those changes to the built-in style.

Cheers,

Daniel

[John Abd-El-Malek]

Thanks for the fast fixes, really appreciate it :)

[Nick Carter]

I'm in the process of rolling new clang-format binaries, I'm noticed that it also will also add newlines to non-inline empty function bodies:

That is, the new clang-format does this:

PanelInformation::~PanelInformation() {

}

instead of this:

PanelInformation::~PanelInformation() {}

Codesearch shows that both styles are equally prevalent today (links: one line has 3845 matches, two lines has 4038 matches). This tooling change will likely swing the balance towards "two lines" as the dominant choice.

Everybody cool?

 - nick 

[Nick Carter]

To be clear, since I mangled my words: the ToT clang-format adds newlines in Chromium mode, even if the {}'s are empty.

[John Abd-El-Malek]

On Fri, Apr 11, 2014 at 3:35 PM, Nick Carter <ni...@chromium.org> wrote:

To be clear, since I mangled my words: the ToT clang-format adds newlines in Chromium mode, even if the {}'s are empty.

On Fri, Apr 11, 2014 at 3:33 PM, Nick Carter <ni...@chromium.org> wrote:

I'm in the process of rolling new clang-format binaries, I'm noticed that it also will also add newlines to non-inline empty function bodies:

That is, the new clang-format does this:

PanelInformation::~PanelInformation() {

}

instead of this:

PanelInformation::~PanelInformation() {}

Codesearch shows that both styles are equally prevalent today (links: one line has 3845 matches, two lines has 4038 matches). This tooling change will likely swing the balance towards "two lines" as the dominant choice.

Everybody cool?

Isn't the first style against the style guide? It says: "The open curly brace is always at the end of the same line as the last parameter.", to me that reads that for non-inlined method definitions, nothing should be after the open curly brace.

[John Abd-El-Malek]

On Fri, Apr 11, 2014 at 3:40 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Apr 11, 2014 at 3:35 PM, Nick Carter <ni...@chromium.org> wrote:

To be clear, since I mangled my words: the ToT clang-format adds newlines in Chromium mode, even if the {}'s are empty.

On Fri, Apr 11, 2014 at 3:33 PM, Nick Carter <ni...@chromium.org> wrote:

I'm in the process of rolling new clang-format binaries, I'm noticed that it also will also add newlines to non-inline empty function bodies:

That is, the new clang-format does this:

PanelInformation::~PanelInformation() {

}

instead of this:

PanelInformation::~PanelInformation() {}

Codesearch shows that both styles are equally prevalent today (links: one line has 3845 matches, two lines has 4038 matches). This tooling change will likely swing the balance towards "two lines" as the dominant choice.

Everybody cool?

Isn't the first style against the style guide? It says: "The open curly brace is always at the end of the same line as the last parameter.", to me that reads that for non-inlined method definitions, nothing should be after the open curly brace.

oops, I mean the "Isn't the second style"

[David Michael]

On Fri, Apr 11, 2014 at 4:40 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Apr 11, 2014 at 3:40 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Apr 11, 2014 at 3:35 PM, Nick Carter <ni...@chromium.org> wrote:

To be clear, since I mangled my words: the ToT clang-format adds newlines in Chromium mode, even if the {}'s are empty.

On Fri, Apr 11, 2014 at 3:33 PM, Nick Carter <ni...@chromium.org> wrote:

I'm in the process of rolling new clang-format binaries, I'm noticed that it also will also add newlines to non-inline empty function bodies:

That is, the new clang-format does this:

PanelInformation::~PanelInformation() {

}

instead of this:

PanelInformation::~PanelInformation() {}

Codesearch shows that both styles are equally prevalent today (links: one line has 3845 matches, two lines has 4038 matches). This tooling change will likely swing the balance towards "two lines" as the dominant choice.

Everybody cool?

Isn't the first style against the style guide? It says: "The open curly brace is always at the end of the same line as the last parameter.", to me that reads that for non-inlined method definitions, nothing should be after the open curly brace.

oops, I mean the "Isn't the second style"

Agreed, I prefer the first style. AIUI, the change will make clang-format start doing the first style, whereas it currently does the second. I think this is an improvement.

[Chris Hopman]

On the next line it says: "The close curly brace is either on the last line by itself or (if other style rules permit) on the same line as the open curly brace."

[Nick Carter]

On Fri, Apr 11, 2014 at 3:40 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Apr 11, 2014 at 3:40 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Apr 11, 2014 at 3:35 PM, Nick Carter <ni...@chromium.org> wrote:

To be clear, since I mangled my words: the ToT clang-format adds newlines in Chromium mode, even if the {}'s are empty.

On Fri, Apr 11, 2014 at 3:33 PM, Nick Carter <ni...@chromium.org> wrote:

I'm in the process of rolling new clang-format binaries, I'm noticed that it also will also add newlines to non-inline empty function bodies:

That is, the new clang-format does this:

PanelInformation::~PanelInformation() {

}

instead of this:

PanelInformation::~PanelInformation() {}

Codesearch shows that both styles are equally prevalent today (links: one line has 3845 matches, two lines has 4038 matches). This tooling change will likely swing the balance towards "two lines" as the dominant choice.

Everybody cool?

Isn't the first style against the style guide? It says: "The open curly brace is always at the end of the same line as the last parameter.", to me that reads that for non-inlined method definitions, nothing should be after the open curly brace.

oops, I mean the "Isn't the second style"

I don't think one-line is prohibited by the language you linked to, nor by the style guide in total, which has a few "good code" example snippets (copied below) that use the one-line style. The codebase being split fifty-fifty is evidence that at best we're tolerant of both, or at worst that we have a population of reviewers insisting on one style or the other, inconsistently.

void Circle::Rotate(double /*radians*/) {}

MyClass::MyClass(int var) : some_var_(var), some_other_var_(var + 1) {}

X::X() : X("") { }  (whoah really? - nick

[John Abd-El-Malek]

On Fri, Apr 11, 2014 at 3:54 PM, Nick Carter <ni...@chromium.org> wrote:

On Fri, Apr 11, 2014 at 3:40 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Apr 11, 2014 at 3:40 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Apr 11, 2014 at 3:35 PM, Nick Carter <ni...@chromium.org> wrote:

To be clear, since I mangled my words: the ToT clang-format adds newlines in Chromium mode, even if the {}'s are empty.

On Fri, Apr 11, 2014 at 3:33 PM, Nick Carter <ni...@chromium.org> wrote:

I'm in the process of rolling new clang-format binaries, I'm noticed that it also will also add newlines to non-inline empty function bodies:

That is, the new clang-format does this:

PanelInformation::~PanelInformation() {

}

instead of this:

PanelInformation::~PanelInformation() {}

Codesearch shows that both styles are equally prevalent today (links: one line has 3845 matches, two lines has 4038 matches). This tooling change will likely swing the balance towards "two lines" as the dominant choice.

Everybody cool?

Isn't the first style against the style guide? It says: "The open curly brace is always at the end of the same line as the last parameter.", to me that reads that for non-inlined method definitions, nothing should be after the open curly brace.

oops, I mean the "Isn't the second style"

I don't think one-line is prohibited by the language you linked to, nor by the style guide in total, which has a few "good code" example snippets (copied below) that use the one-line style. The codebase being split fifty-fifty is evidence that at best we're tolerant of both, or at worst that we have a population of reviewers insisting on one style or the other, inconsistently.

void Circle::Rotate(double /*radians*/) {}

MyClass::MyClass(int var) : some_var_(var), some_other_var_(var + 1) {}

X::X() : X("") { }  (whoah really? - nick

 

ah, ok good to know. Note I don't care either way :)

[Antoine Labour]

On Fri, Apr 11, 2014 at 3:35 PM, Nick Carter <ni...@chromium.org> wrote:

To be clear, since I mangled my words: the ToT clang-format adds newlines in Chromium mode, even if the {}'s are empty.

That would be particularly upsetting for at least inline empty functions in header files. 

Use case:

class FooDelegate: {

 public:

  virtual A() {}

  virtual B() {}

  virtual C() {}

};

Compared to:

class FooDelegate: {

 public:

  virtual A() {

  }

  virtual B() {

  }

  virtual C() {

  }

};

Antoine

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Nico Weber]

On Fri, Apr 11, 2014 at 4:37 PM, Antoine Labour <pi...@google.com> wrote:

On Fri, Apr 11, 2014 at 3:35 PM, Nick Carter <ni...@chromium.org> wrote:

To be clear, since I mangled my words: the ToT clang-format adds newlines in Chromium mode, even if the {}'s are empty.

That would be particularly upsetting for at least inline empty functions in header files. 

Use case:

class FooDelegate: {

 public:

  virtual A() {}

  virtual B() {}

  virtual C() {}

};

Compared to:

class FooDelegate: {

 public:

  virtual A() {

  }

  virtual B() {

  }

  virtual C() {

  }

};

I think Nick's note was only for out-of-line functions, not for inline class methods. Those should still be formatted un-upsettingly ("downsettingly"?)

[Nick Carter]

The new version of clang-format will put these examples on one line, as you prefer, because they're inline. It's non-inline functions that are affected.

On Fri, Apr 11, 2014 at 4:37 PM, Antoine Labour <pi...@google.com> wrote:

[Antoine Labour]

On Fri, Apr 11, 2014 at 4:44 PM, Nick Carter <ni...@chromium.org> wrote:

The new version of clang-format will put these examples on one line, as you prefer, because they're inline. It's non-inline functions that are affected.

I see. Then I don't care much (aside of the churn).

[Nico Weber]

On Fri, Apr 11, 2014 at 5:15 PM, Antoine Labour <pi...@google.com> wrote:

On Fri, Apr 11, 2014 at 4:44 PM, Nick Carter <ni...@chromium.org> wrote:

The new version of clang-format will put these examples on one line, as you prefer, because they're inline. It's non-inline functions that are affected.

I see. Then I don't care much (aside of the churn).

`git cl format` only formats lines touched by a CL, so this hopefully won't cause a ton of churn.

[Antoine Labour]

On Fri, Apr 11, 2014 at 5:19 PM, Nico Weber <tha...@chromium.org> wrote:

On Fri, Apr 11, 2014 at 5:15 PM, Antoine Labour <pi...@google.com> wrote:

On Fri, Apr 11, 2014 at 4:44 PM, Nick Carter <ni...@chromium.org> wrote:

The new version of clang-format will put these examples on one line, as you prefer, because they're inline. It's non-inline functions that are affected.

I see. Then I don't care much (aside of the churn).

`git cl format` only formats lines touched by a CL, so this hopefully won't cause a ton of churn.

There are parts of the tree that mandate (through presubmit) that files are clang-formatted.

Antoine

[Thiago Farina]

On Fri, Apr 11, 2014 at 8:44 PM, Nick Carter <ni...@chromium.org> wrote:

The new version of clang-format will put these examples on one line, as you prefer, because they're inline. It's non-inline functions that are affected.

Nick, Daniel, clang-format also has issues when formatting ternary operator ?:

https://codereview.chromium.org/228783002/diff/80001/chrome/browser/favicon/favicon_handler.cc

Look at line 218.

I'm sure Peter would prefer the diff to stay formatted as it is in the red diff. 

Sorry for reporting this here in this thread though.

--

Thiago Farina

[Daniel Cheng]

While the codebase has more examples of placing : on the previous line with a ?:

$ git gs "?.\+:$" | grep -v third_party | wc -l

732

$ git gs "^ \+?.*[^:]$" | grep -v third_party | wc -l

362

I'm not a fan of the original style--I'd rather see the : on the beginning of the next line and aligned with the ? to make it more obvious what's going on if it needs to be wrapped.

Daniel

--

[Daniel Jasper]

I implemented the option "BreakBeforeTernaryOperators" about half a year ago, but apparently forgot to change the default for Chromium (or at least start a discussion about it). If you come to a conclusion, you can put a corresponding setting in Chromium's .clang-format file and I'll then port that back to the default settings for the built-in Chromium style.

[Peter Kasting]

On Fri, Apr 11, 2014 at 3:33 PM, Nick Carter <ni...@chromium.org> wrote:

I'm in the process of rolling new clang-format binaries, I'm noticed that it also will also add newlines to non-inline empty function bodies:

That is, the new clang-format does this:

PanelInformation::~PanelInformation() {

}

instead of this:

PanelInformation::~PanelInformation() {}

Good; for out-of-line functions, I think this is more logically consistent with how we've interpreted other rules.  I didn't realize the second style was so prevalent; in code I work in I've mostly tried to enforce the first style, though because it's a minor issue I've tried not to ask people to go around fixing existing instances.

PK

[Peter Kasting]

On Sun, Apr 13, 2014 at 4:34 AM, Daniel Jasper <dja...@google.com> wrote:

I implemented the option "BreakBeforeTernaryOperators" about half a year ago, but apparently forgot to change the default for Chromium (or at least start a discussion about it). If you come to a conclusion, you can put a corresponding setting in Chromium's .clang-format file and I'll then port that back to the default settings for the built-in Chromium style.

I know I've raised the issue of ternary formatting twice before -- it's long been my biggest objection to clang-format's formatting -- so it would be nice to make clang-format do the right thing.  Could you just go ahead and make the change without a third discussion on it?

To be clear, the setting should be such that we progressively wrap like this:

a ? b : c;

a ?

    b : c;

a ?

    b :

    c;

The rationale for this is that this is not only the prevalent style (indeed, before clang-format began being used at all, it was nearly-universal), it's more consistent with the style guide rule about placing operators on the ends of lines.

PK

[Peter Kasting]

Here's another thing I noticed lately that seems questionable.  When attempting to wrap a >80-char line with a bunch of dot and arrow operators:

a->b.c().d()->e().f.g->h;

Instead of breaking after an operator ("operators go on the ends of lines") and as few times as necessary to wrap ("minimize vertical whitespace"):

a->b.c().d()->

    e().f.g->h;

clang-format did this:

a

    ->b

    .c()

    .d()

    ->e()

    .f

    .g

    ->h;

This seems like both an atypical style for Chromium code and not in the spirit of the two style rules I noted above.  Besides this, I personally find it pretty hard to read due to the massive increase in vertical space.

PK

[Nico Weber]

I think this has been discussed extensively for the google c++ style guide, and clang-format's behavior is what has been ruled as "correct" (even though it's not super common internally either). We try not to differ from google-internal formatting more than necessary.

 

PK

[Nico Weber]

I think that the reasoning for this is that "().foo" and "()->foo" is categorized as a "builder call", which is somewhat common:

  MyClass a = MyClassBuilder()

                  .WithFoo(1)

                  .WithBar(2)

                  .WithLongParameterThing(4)

                  .WithAnotherLongParameterThing(4)

                  .Build();

which is nicer with one line per parameter. (`a->b.c().d()->e().f.g->h;` is formatted as just one line over here, so I'm guessing your example is slightly different.)

 

PK

[Peter Kasting]

On Mon, Apr 14, 2014 at 12:16 PM, Nico Weber <tha...@chromium.org> wrote:

I think this has been discussed extensively for the google c++ style guide, and clang-format's behavior is what has been ruled as "correct" (even though it's not super common internally either). We try not to differ from google-internal formatting more than necessary.

That sentence could be stated about literally every difference between "Chromium" clang-format rules and "standard" clang-format rules.  Indeed, it was the reason there was initially pushback against making a Chromium-specific styling mode even when the Chromium codebase was very consistent about doing something different than the clang-format default.  It seems like more recently we've finally begun agreeing that, where Chromium code tries to be consistent about doing something else, and that something is not in violation of the internal guide, we should make clang-format preserve that style in Chromium code.  I would like that applied here as well.

Incidentally, feel free to link me to the C-style discussion that ruled this style superior internally.  I would appreciate reading the rationale.

PK

[Peter Kasting]

What are the rules that trigger this "builder call" heuristic?  I don't understand the distinction between "builder" and "non-builder" call chains -- simply that an assignment is involved? This distinction felt very arbitrary and out-of-place in the code review I was doing where I noticed this.  Here's an example:

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/autofill/autofill_interactive_uitest.cc#newcode177

IMO in this case clang-format shouldn't have been redoing the line-wrapping so extensively.

PK

[Daniel Cheng]

​I couldn't find internal mailing list discussion, but ​you're likely interested in https://b.corp.google.com/issue?id=8831492 (builders/method chains), https://b.corp.google.com/issue?id=8071453 (arrow wrapping), and https://sites.google.com/a/google.com/llvm/clang-tools/clang-format/faq#TOC-Why-does-clang-format-put---onto-the-new-line- (arrow wrapping again) -- sorry, these links will only work for Googlers.

​As for "operators go on the ends of lines", I think it's worth pointing out that there are already two pretty consistent exceptions to this rule. Both << and . tend to be placed at the start of the line rather than the end when wrapping is required.​ Semantically, <- and . are pretty close in meaning... so at least for me, it doesn't make sense to be inconsistent between the two. Either we should place both at the end of a line or both at the beginning.

I don't know what heuristics we use to detect builders, but I would suspect it's triggered on some combination of > n chained methods where each method has <= 1 parameter.​

Daniel

On Mon, Apr 14, 2014 at 12:14 PM, Peter Kasting <pkas...@chromium.org> wrote:

--

[Chris Hopman]

On Mon, Apr 14, 2014 at 12:22 PM, Peter Kasting <pkas...@chromium.org> wrote:

It seems like more recently we've finally begun agreeing that, where Chromium code tries to be consistent about doing something else, and that something is not in violation of the internal guide, we should make clang-format preserve that style in Chromium code.  

I think we should have a clear (and, preferably, high) bar for imposing our personal style preferences on clang-format and chromium.

Personally, I'd go so far as saying that if it's not completely obvious that Chrome has already decided on a particular style, we should be accepting the default clang-format style (or convincing them that what they are doing is wrong). It makes everyone's life easier. Examples of things that would be completely obvious: it's in Chromium's style guide, a significant majority of instances in Chrome do it a certain way, etc.

Side note: could we maybe create a different group for these c-style discussions? And then better document decisions that come of them? Having the discussions go to both lists would even work for me. I think it is very hostile to new, and even relatively experienced, devs to have the decisions that come of these discussions completely hidden among the mass of chromium-dev threads. It leads the decisions being inconsistently enforced and I don't think we should have codereview discussions like:

reviewer: don't do this, do this other thing

author: ok sure. Is there anywhere I can read more about why?

reviewer: there's a chromium-dev thread from a couple years ago about it

(author goes off and searches for "refcounted" on chromium-dev and finds ~2 weeks worth of reading material)

[Peter Kasting]

​As for "operators go on the ends of lines", I think it's worth pointing out that there are already two pretty consistent exceptions to this rule. Both << and . tend to be placed at the start of the line rather than the end when wrapping is required.

The style guide explicitly says that << should be placed at the start of the line for logging statements, and that that is an exception of the standard rule.  So that covers that case.

The dot operator is not called out similarly, and therefore IMO placing it at the beginning of a line is not correct.

Semantically, <- and . are pretty close in meaning... so at least for me, it doesn't make sense to be inconsistent between the two.

I agree.

Either we should place both at the end of a line or both at the beginning.

Given my statement above, I think it's obvious I fall into the "these also need to be at the end of a line" camp.

PK

[Daniel Cheng]

Here are some numbers from two years ago (arbitrarily chosen, hopefully mostly pre clang-format) and today-ish for the dot operator:

# Two years ago

$ git gs "^ \+\." | grep "\.cc:" | grep -v "//" | grep -v "third_party/" | grep -v " */\*" | grep -v " *\*" | wc -l  # Leading .

4491

$ git gs "\.$" | grep "\.cc:" | grep -v "//" | grep -v "third_party/" | grep -v " */\*" | grep -v " *\*" | wc -l  # Trailing .

359

# Today

$ git gs "^ \+\." | grep "\.cc:" | grep -v "//" | grep -v "third_party/" | grep -v " */\*" | grep -v " *\*" | wc -l  # Leading .

8175

$ git gs "\.$" | grep "\.cc:" | grep -v "//" | grep -v "third_party/" | grep -v " */\*" | grep -v " *\*" | wc -l  # Trailing .

546

Most of the examples with a leading dot appear to be correctly matched (I only skimmed, as there were too many to really look over here), while there are some false positives in the trailing dot search (mostly from a poorly-formatted comment block in condition_variable_win.cc).

The arrow operator is wrapped a lot less. Two years ago there were 65 instances at the start of the line and 838 at the end. Today, there are 915 instances at the start of the line, 2041 at the end.

Daniel

[Peter Kasting]

On Mon, Apr 14, 2014 at 1:19 PM, Chris Hopman <cjho...@chromium.org> wrote:

On Mon, Apr 14, 2014 at 12:22 PM, Peter Kasting <pkas...@chromium.org> wrote:

It seems like more recently we've finally begun agreeing that,  about doing something else, and that something is not in violation of the internal guide, we should make clang-format preserve that style in Chromium code.  

I think we should have a clear (and, preferably, high) bar for imposing our personal style preferences on clang-format and chromium.

Personally, I'd go so far as saying that if it's not completely obvious that Chrome has already decided on a particular style, we should be accepting the default clang-format style (or convincing them that what they are doing is wrong). It makes everyone's life easier. Examples of things that would be completely obvious: it's in Chromium's style guide, a significant majority of instances in Chrome do it a certain way, etc.

You're saying the same thing I'm saying, but in more words.  Me: "where Chromium code tries to be consistent", you: "a significant majority of instances in Chrome do it a certain way".

That would be why, every time I've cited the ternary formatting issue, I've taken pains to note that outside of clang-format-introduced exceptions, Chromium code has been largely consistent in formatting things the way I've been asking.  I believe that, even _with_ the clang-format-introduced exceptions, we still meet the "significant majority" bar you propose.

Side note: could we maybe create a different group for these c-style discussions?

Honestly, these happen rarely.  And I don't consider this to be a discussion thread about how Chromium code should be formatted, so much as what cases exist where clang-format reformats existing legal Chromium code to some other, much less typical style.

PK

[Peter Kasting]

On Mon, Apr 14, 2014 at 5:35 PM, Daniel Cheng <dch...@chromium.org> wrote:

Here are some numbers from two years ago (arbitrarily chosen, hopefully mostly pre clang-format) and today-ish for the dot operator:

To be clear, unlike with ternary wrapping, I'm not accusing clang-format of having introduced some kind of style to the Chromium codebase w.r.t. breaking before dots that rarely occurred before.

I'm just saying that I recall nothing in the Google style guide or Chromium style guide that calls out any operator other than ">>", and so like you I think "." and "->" should be formatted consistently, and the plain reading of the style guide suggests placing them at the ends of lines.  It looks from your numbers like we used to be very consistent about that for ->, and very consistent about not doing that for ., so we were consistently inconsistent (ugh).

PK

[Daniel Jasper]

On Mon, Apr 14, 2014 at 9:09 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Sun, Apr 13, 2014 at 4:34 AM, Daniel Jasper <dja...@google.com> wrote:

I implemented the option "BreakBeforeTernaryOperators" about half a year ago, but apparently forgot to change the default for Chromium (or at least start a discussion about it). If you come to a conclusion, you can put a corresponding setting in Chromium's .clang-format file and I'll then port that back to the default settings for the built-in Chromium style.

I know I've raised the issue of ternary formatting twice before -- it's long been my biggest objection to clang-format's formatting -- so it would be nice to make clang-format do the right thing.  Could you just go ahead and make the change without a third discussion on it?

Sure, all I ask is that you check it in to Chromium's .clang-format file to have a basic decision process behind such changes. I'll port everything I find there back as soon as I can.

To be clear, the setting should be such that we progressively wrap like this:

a ? b : c;

a ?

    b : c;

clang-format currently doesn't do that as commonly an extra line break improves the readability sufficiently to be worth it (after all, this is a multiline ternary expression already and the ":" can somewhat blur together with "a"). So clang-format would currently immediately switch to option 3 below. There is an open bug to not do this if "b" and "c" are simple, e.g. literals or bare identifiers.

Dependent on token lengths, there is another option:

a ? b :

    c;

Here, all operators are still spatially isolated, so an extra line break does not really improve readability.

a ?

    b :

    c;

The rationale for this is that this is not only the prevalent style (indeed, before clang-format began being used at all, it was nearly-universal), it's more consistent with the style guide rule about placing operators on the ends of lines.

I am not getting into this argument again. The option is implemented, so we have put in the effort and I am happy to set this as desired for Chromium style.

PK

[Vincent Scheib]

On Mon, Apr 14, 2014 at 1:19 PM, Chris Hopman <cjho...@chromium.org> wrote:

On Mon, Apr 14, 2014 at 12:22 PM, Peter Kasting <pkas...@chromium.org> wrote:

It seems like more recently we've finally begun agreeing that, where Chromium code tries to be consistent about doing something else, and that something is not in violation of the internal guide, we should make clang-format preserve that style in Chromium code.  

I think we should have a clear (and, preferably, high) bar for imposing our personal style preferences on clang-format and chromium.

+1. I'm happy to see this thread, but it seems to have run off into the weeds. The value of easy to use consistent automatic formatting is higher than debating how we mostly-ish used a given pattern of indents. The value is in consistency and ease of use, not chromium flavors.

Personally, I'd go so far as saying that if it's not completely obvious that Chrome has already decided on a particular style, we should be accepting the default clang-format style (or convincing them that what they are doing is wrong). It makes everyone's life easier. Examples of things that would be completely obvious: it's in Chromium's style guide, a significant majority of instances in Chrome do it a certain way, etc.

Side note: could we maybe create a different group for these c-style discussions? And then better document decisions that come of them? Having the discussions go to both lists would even work for me. I think it is very hostile to new, and even relatively experienced, devs to have the decisions that come of these discussions completely hidden among the mass of chromium-dev threads. It leads the decisions being inconsistently enforced and I don't think we should have codereview discussions like:

reviewer: don't do this, do this other thing

author: ok sure. Is there anywhere I can read more about why?

reviewer: there's a chromium-dev thread from a couple years ago about it

(author goes off and searches for "refcounted" on chromium-dev and finds ~2 weeks worth of reading material)

--

[Nick Carter]

On Tue, Apr 15, 2014 at 4:51 AM, Daniel Jasper <dja...@google.com> wrote:

On Mon, Apr 14, 2014 at 9:09 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Sun, Apr 13, 2014 at 4:34 AM, Daniel Jasper <dja...@google.com> wrote:

I implemented the option "BreakBeforeTernaryOperators" about half a year ago, but apparently forgot to change the default for Chromium (or at least start a discussion about it). If you come to a conclusion, you can put a corresponding setting in Chromium's .clang-format file and I'll then port that back to the default settings for the built-in Chromium style.

I know I've raised the issue of ternary formatting twice before -- it's long been my biggest objection to clang-format's formatting -- so it would be nice to make clang-format do the right thing.  Could you just go ahead and make the change without a third discussion on it?

Sure, all I ask is that you check it in to Chromium's .clang-format file to have a basic decision process behind such changes. I'll port everything I find there back as soon as I can.

I'll take care of moving forward the .clang-format file update. My process for updating clang-format binaries includes crunching stats by running the new and old versions on every change committed in the previous month, to get a sense for the level of tool adoption and style thrash. I can do that for this sort of change too, and will share what I find back with this group.

[Peter Kasting]

On Tue, Apr 15, 2014 at 7:06 AM, Vincent Scheib <sch...@chromium.org> wrote:

On Mon, Apr 14, 2014 at 1:19 PM, Chris Hopman <cjho...@chromium.org> wrote:

On Mon, Apr 14, 2014 at 12:22 PM, Peter Kasting <pkas...@chromium.org> wrote:

It seems like more recently we've finally begun agreeing that, where Chromium code tries to be consistent about doing something else, and that something is not in violation of the internal guide, we should make clang-format preserve that style in Chromium code.  

I think we should have a clear (and, preferably, high) bar for imposing our personal style preferences on clang-format and chromium.

+1. I'm happy to see this thread, but it seems to have run off into the weeds. The value of easy to use consistent automatic formatting is higher than debating how we mostly-ish used a given pattern of indents. The value is in consistency and ease of use, not chromium flavors.

And I have long said that if we want to run clang-format over the whole codebase, and then make it a mandatory (and automatic) part of submission, then I don't care what kind of formatting it does.  (Personally, I think this would be the best possible route forward; however, no one seems to want to bother.)

The problem occurs when the codebase is mostly not clang-formatted, and the clang-format formatting is a sharp change from what the rest of the codebase is doing.  In that case there is no "consistency" advantage.  If we want consistency, we need to either not use clang-format, use it everywhere, or continue the current pattern of having some people use it some places but ensure that the tools' output matches existing code -- yes, including patterns of indents.

PK

[Nico Weber]

On Tue, Apr 15, 2014 at 12:49 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Apr 15, 2014 at 7:06 AM, Vincent Scheib <sch...@chromium.org> wrote:

On Mon, Apr 14, 2014 at 1:19 PM, Chris Hopman <cjho...@chromium.org> wrote:

On Mon, Apr 14, 2014 at 12:22 PM, Peter Kasting <pkas...@chromium.org> wrote:

It seems like more recently we've finally begun agreeing that, where Chromium code tries to be consistent about doing something else, and that something is not in violation of the internal guide, we should make clang-format preserve that style in Chromium code.  

I think we should have a clear (and, preferably, high) bar for imposing our personal style preferences on clang-format and chromium.

+1. I'm happy to see this thread, but it seems to have run off into the weeds. The value of easy to use consistent automatic formatting is higher than debating how we mostly-ish used a given pattern of indents. The value is in consistency and ease of use, not chromium flavors.

And I have long said that if we want to run clang-format over the whole codebase, and then make it a mandatory (and automatic) part of submission, then I don't care what kind of formatting it does.  (Personally, I think this would be the best possible route forward; however, no one seems to want to bother.)

The problem occurs when the codebase is mostly not clang-formatted, and the clang-format formatting is a sharp change from what the rest of the codebase is doing.

I think most others don't perceive the clang-format output as sharply different.

 

In that case there is no "consistency" advantage.  If we want consistency, we need to either not use clang-format, use it everywhere, or continue the current pattern of having some people use it some places but ensure that the tools' output matches existing code -- yes, including patterns of indents.

Possible futures:

1. Reformat whole codebase. Pro: Could switch to the regular google style guide. Cons: Blame output becomes unusable. Also, since clang-format changes every now and then, we wouldn't have the forever-consistency you seem to desire (or we'd have to do global reformats frequently.)

2. Keep codebase as-is, let people format code how they want, but add a rule "reviewers must not leave style nits on clang-formatted changes". Pro: Most people don't feel more strongly about getting their stuff done than formatting (or so I claim), so most people would be happy about getting their code in more quickly. Cons: Requires clang-format to be "good enough".

3. Keep codebase as-is, don't say anything. Some people will use clang-format locally as it formats faster/better on average than they'd format manually. Still allow reviewers-dependent formatting-related comments on all cls, independent of how they were formatted. Pro: None? Cons: Code reviews still contain formatting nits in comments.

4. Like 2, but informal: Reviewers who think clang-format formats "good enough" declare they're going to not leave formatting nits on clang-formatted code; authors pick reviewers based on if they like style nits on their CLs or not.

Option 4 is a bit of a caricature, but I think that's what's happening today. I think 2 is the most likely end state.

 

PK

[Peter Kasting]

On Tue, Apr 15, 2014 at 1:13 PM, Nico Weber <tha...@chromium.org> wrote:

On Tue, Apr 15, 2014 at 12:49 PM, Peter Kasting <pkas...@chromium.org> wrote:

The problem occurs when the codebase is mostly not clang-formatted, and the clang-format formatting is a sharp change from what the rest of the codebase is doing.

I think most others don't perceive the clang-format output as sharply different.

I think that statement would be true irrespective of clang-format's output.  I believe the majority of developers simply are not hyper-aware of style issues to begin with.  That's not a value judgment.  Honestly, my life would be easier if I wasn't cursed with near-OCD about style issues.

1. Reformat whole codebase. Pro: Could switch to the regular google style guide. Cons: Blame output becomes unusable.

FWIW, I have never been sympathetic to the "you broke my blame" argument.  It just means one extra step in tracing back past such changes.  Over time, as we get further on from the change, that happens less and less.  I'd prefer to optimize for the readability of code for the infinite future, not the ever-lessening hit to convenience now.

 

Also, since clang-format changes every now and then, we wouldn't have the forever-consistency you seem to desire (or we'd have to do global reformats frequently.)

That's a much more compelling reason not to do this.  That said, we've tried in the past to limit the number and scope of global style changes.  Assuming that remains reasonably limited, and each individual style change that does happen would presumably affect a tiny fraction of the whole codebase, I still think this is potentially feasible.

2. Keep codebase as-is, let people format code how they want, but add a rule "reviewers must not leave style nits on clang-formatted changes".

And how do we know for sure if a change is due to clang-format or the coder's own desires?  It seems like unless we mark up each CL with whether clang-format was run on it, or say that reviewers must never leave style nits period, this would be impossible to enforce.

 

Pro: Most people don't feel more strongly about getting their stuff done than formatting (or so I claim), so most people would be happy about getting their code in more quickly. Cons: Requires clang-format to be "good enough".

Which is precisely why threads like this exist: I don't want to feel the need to override clang-format changes any more than other people do, hence I try to advocate for clang-format's output being "good enough".

What's frustrating about this is when the same people who don't really mind whatever clang-format decides to do to begin with object to those few of us who _do_ care raising issues with what it currently does.

PK