clang-format chromium ternary operator

Hi,

Looks like clang-format for Chromium style is incorrectly formatting
ternary operators (?:slight_smile:

By Google C++ Style guide, which Chromium code follows:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolean_Expressions#Boolean_Expressions

The operators should be in the end of the expressions.

But clang-format did this:
https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/location_bar/location_bar_view.cc#newcode1107

According to the style guide and Peter it should have been formatted like this:

return (ui::GetDisplayLayout() == ui::LAYOUT_TOUCH) ?
    GetItemPadding() / 2 : 0;

This was decided to be the best way to go forward with Google’s C++ style. I am reasonably certain that the style guide does not contain anything that says otherwise. The section you quote:
a) applies to boolean expressions, which a conditional expression is not.
b) explicitly says that you can also wrap them to the new line.

If you find anything else in the style guide, please let me know, I am happy to get that changed.

As for “This is not done in Chromium”:
https://code.google.com/p/chromium/codesearch#search/&q=%5E%5C%20*%5C?&all=1&sq=package:chromium&type=cs

and
https://code.google.com/p/chromium/codesearch#search/&q=(?m:%5C?.%5Cn%5Cs%5C:)%20pcre:yes&all=1&sq=package:chromium&type=cs

I am aware that the other way of formatting is ~2:1 more common in Chromium right now, but there already is quite a bit of diversity.

The basic reasoning for going with the slightly less popular style is this very basic example:

aaaaa ? bbbbb :
ccccc;

aaaaa ? bbbbb
: ccccc;

Here, the second formatting looks more structured and is easier to grasp at first sight. If the operands get more complex, this gets even more obvious. And for consistency with that formatting, ? and : should both be wrapped to the new line where necessary.

Cheers,
Daniel

This was decided to be the best way to go forward with Google's C++ style.
I am reasonably certain that the style guide does not contain anything that
says otherwise. The section you quote:
a) applies to boolean expressions, which a conditional expression is not.
b) explicitly says that you can also wrap them to the new line.

If you find anything else in the style guide, please let me know, I am
happy to get that changed.

The Google style guide used to explicitly ban wrapping before an operator
(not just a boolean operator, but any operator) rather than after. IIRC it
no longer does so, but Chrome code generally dates from before this change
and is very consistent about this wrapping, not just for ?:, but
importantly for other operators as well (where we also need consistency).

As for "This is not done in Chromium":

https://code.google.com/p/chromium/codesearch#search/&q=^\%20*\?&all=1&sq=package:chromium&type=cs
and

https://code.google.com/p/chromium/codesearch#search/&q=(?m:\?.*\n\s*\:)%20pcre:yes&all=1&sq=package:chromium&type=cs

Your search includes things like v8 and third_party, which have their own
conventions and are not "Chromium" code. Restrict to Chromium code, and I
suspect the ratio will change.

The basic reasoning for going with the slightly less popular style is this

very basic example:

aaaaa ? bbbbb :
        ccccc;

aaaaa ? bbbbb
      : ccccc;

Here, the second formatting looks more structured and is easier to grasp
at first sight. If the operands get more complex, this gets even more
obvious. And for consistency with that formatting, ? and : should both be
wrapped to the new line where necessary.

The Google style guide generally directs line wrapping to begin at a
4-space indent. Indenting even with something on a previous line is
generally used for lines of function arguments. So the correct way of
formatting the first example would be:

aaaaa ? bbbbb :
    ccccc;

...which is generally less appealing than

aaaaa ?
    bbbbb : ccccc;

...which is why most Chrome code that needs to linebreak these at all does
it this way. Whereas your indenting is not only unusual for breaking
before an operator, it's also unusual for indenting even.

Finally, regardless of all other arguments, and even ignoring the fact that
the above codesearch links included code they shouldn't: there's a way of
formatting these in Chromium code that is clearly more common even by a
pessimistic search, and is clearly compliant with the style guide.
Therefore, the auto-formatter for Chromium code should use this pattern.
It doesn't matter if your way is legal or not, the most important rule in
the whole style guide is "be consistent", which this does not do as well as
it could.

PK

This was decided to be the best way to go forward with Google's C++
style. I am reasonably certain that the style guide does not contain
anything that says otherwise. The section you quote:
a) applies to boolean expressions, which a conditional expression is not.
b) explicitly says that you can also wrap them to the new line.

If you find anything else in the style guide, please let me know, I am
happy to get that changed.

The Google style guide used to explicitly ban wrapping before an operator
(not just a boolean operator, but any operator) rather than after. IIRC it
no longer does so, but Chrome code generally dates from before this change
and is very consistent about this wrapping, not just for ?:, but
importantly for other operators as well (where we also need consistency).

The Chromium style guide explicitly says: "Coding style for the Chromium
projects generally follows the Google C++ Style Guide. The notes below are
usually just extensions beyond what the Google style guide already says. If
this document doesn't mention a rule, follow the Google C++ style.". IMO,
this includes changes to the style guide (which happen happen frequently).

As for "This is not done in Chromium":

https://code.google.com/p/chromium/codesearch#search/&q=^\%20*\?&all=1&sq=package:chromium&type=cs
and

https://code.google.com/p/chromium/codesearch#search/&q=(?m:\?.*\n\s*\:)%20pcre:yes&all=1&sq=package:chromium&type=cs

Your search includes things like v8 and third_party, which have their own
conventions and are not "Chromium" code. Restrict to Chromium code, and I
suspect the ratio will change.

Fair enough. I have never worked on the Chromium codebase, so I don't know
which parts follow the style guide and which parts to disregard.

The basic reasoning for going with the slightly less popular style is this

very basic example:

aaaaa ? bbbbb :
        ccccc;

aaaaa ? bbbbb
      : ccccc;

Here, the second formatting looks more structured and is easier to grasp
at first sight. If the operands get more complex, this gets even more
obvious. And for consistency with that formatting, ? and : should both be
wrapped to the new line where necessary.

The Google style guide generally directs line wrapping to begin at a
4-space indent. Indenting even with something on a previous line is
generally used for lines of function arguments. So the correct way of
formatting the first example would be:

aaaaa ? bbbbb :
    ccccc;

...which is generally less appealing than

aaaaa ?
    bbbbb : ccccc;

...which is why most Chrome code that needs to linebreak these at all does
it this way. Whereas your indenting is not only unusual for breaking
before an operator, it's also unusual for indenting even.

That is not correct. The style guide says at a few very specific places
that you have to indent four spaces. However, it does not even say 4 spaces
with respect to what in all but simple cases. E.g. clang-format (and many
other people - e.g. search for && at the end of a line), align the two
operands of a binary expression no matter what. Again, this has been
developed in cooperation with the style arbiters and a LOT of users.

Your own examples clarify why this is a good choice.

Finally, regardless of all other arguments, and even ignoring the fact that

the above codesearch links included code they shouldn't: there's a way of
formatting these in Chromium code that is clearly more common even by a
pessimistic search, and is clearly compliant with the style guide.
Therefore, the auto-formatter for Chromium code should use this pattern.
It doesn't matter if your way is legal or not, the most important rule in
the whole style guide is "be consistent", which this does not do as well as
it could.

I understand your argument and it is perfectly valid. However, also
consider other sorts of consistency: E.g. there are a lot of users that
have to develop in both Chromium- and Google-style code. For them, any
(unnecessary) inconsistency is harmful.

I personally don't have any strong feelings about this (I for one would be
happy with disallowing all multi-line conditional expressions). The
decision to go this way is mostly that complex conditional expressions need
as much structure as they can get. I know that we have other Chromium
engineers that are happy enough with this. Is there a decision making
process for Chromium style?

Cheers,
Daniel

PK

Finally, regardless of all other arguments, and even ignoring the fact

that the above codesearch links included code they shouldn't: there's a way
of formatting these in Chromium code that is clearly more common even by a
pessimistic search, and is clearly compliant with the style guide.
Therefore, the auto-formatter for Chromium code should use this pattern.
It doesn't matter if your way is legal or not, the most important rule in
the whole style guide is "be consistent", which this does not do as well as
it could.

I understand your argument and it is perfectly valid. However, also
consider other sorts of consistency: E.g. there are a lot of users that
have to develop in both Chromium- and Google-style code. For them, any
(unnecessary) inconsistency is harmful.

It seems clearly far more important that Chromium code be internally
consistent than that the formatter begin preferring a new, less-used style
over an existing, common, explicitly-valid style regardless of which way is
common in Google internal code. This is true within other Google projects
as well.

I personally don't have any strong feelings about this (I for one would be

happy with disallowing all multi-line conditional expressions). The
decision to go this way is mostly that complex conditional expressions need
as much structure as they can get. I know that we have other Chromium
engineers that are happy enough with this. Is there a decision making
process for Chromium style?

I don't see what "structure" you're buying with this style; it seems like
this:

aaaaa ?
    bbbbb :
    ccccc;

...is just as "structured" as what you're asking for, but aligns with the
common Chromium idiom.

If you are not willing to accept my argument on its face and simply make
this change, the next step is to escalate to chromium-dev.

But in the case where you "don't have any strong feelings" and I do, and
I've been working on the codebase for 7 years now, and you accept my
argument that the style I'm asking for is valid, I don't see what you gain
by doing so.

PK

Finally, regardless of all other arguments, and even ignoring the fact

that the above codesearch links included code they shouldn't: there's a way
of formatting these in Chromium code that is clearly more common even by a
pessimistic search, and is clearly compliant with the style guide.
Therefore, the auto-formatter for Chromium code should use this pattern.
It doesn't matter if your way is legal or not, the most important rule in
the whole style guide is "be consistent", which this does not do as well as
it could.

I understand your argument and it is perfectly valid. However, also
consider other sorts of consistency: E.g. there are a lot of users that
have to develop in both Chromium- and Google-style code. For them, any
(unnecessary) inconsistency is harmful.

It seems clearly far more important that Chromium code be internally
consistent than that the formatter begin preferring a new, less-used style
over an existing, common, explicitly-valid style regardless of which way is
common in Google internal code. This is true within other Google projects
as well.

I personally don't have any strong feelings about this (I for one would be

happy with disallowing all multi-line conditional expressions). The
decision to go this way is mostly that complex conditional expressions need
as much structure as they can get. I know that we have other Chromium
engineers that are happy enough with this. Is there a decision making
process for Chromium style?

I don't see what "structure" you're buying with this style; it seems like
this:

aaaaa ?
    bbbbb :
    ccccc;

...is just as "structured" as what you're asking for, but aligns with the
common Chromium idiom.

If you are not willing to accept my argument on its face and simply make
this change, the next step is to escalate to chromium-dev.

But in the case where you "don't have any strong feelings" and I do, and
I've been working on the codebase for 7 years now, and you accept my
argument that the style I'm asking for is valid, I don't see what you gain
by doing so.

I told Daniel that we generally follow google-internal style unless there
are strong reasons not to. This issue has been discussed at length for
google-internal style, and I personally don't see a strong reason to
deviate here.

Nico

"All the existing code does it another way" is a strong enough reason to me.

I continue to believe that the Chromium-style-specific formatter should not
wrap this way. I also believe this about any other style question where
there is a consistent pattern within the Chrome codebase, regardless of how
many options are legal.

PK

As for structure: With simple cases and all operands having the same length, it always looks structured. But consider the case from the original code review:

return (ui::GetDisplayLayout() == ui::LAYOUT_TOUCH) ?
    GetItemPadding() / 2 : 0;

To break this discussion. What about making this an option. Both are definitely not uncommon cases, and I am sure people will have different options.

In terms of the default for Chromium, I would suggest to default to what
today is the more common practice. Changing this practice is than a policy decision which seems to be best communicated (to both clang-format and none clang-format users), by suggesting an explicit style guide addition on their mailing list either by you or by people in the Chromium community who care. If the explicit style guide change has been discussed and committed, switching the Chromium style in clang-format becomes a no-brainer.

Cheers,
Tobi

"All the existing code does it another way" is a strong enough reason to
me.

It would be, but that is simply not the case. It is hard to come up with
good numbers, but e.g. compare the numbers for ":" on the new and old lines:

https://code.google.com/p/chromium/codesearch#search/&q=(?m:\?.*\n\%20*\:)%20-file:v8%20-file:usr/include%20lang:cc%20pcre:yes&all=1&sq=package:chromium&type=cs

https://code.google.com/p/chromium/codesearch#search/&q=(?m:\?\n{0,1}.*\%20\:$)%20-file:v8%20-file:usr/include%20lang:cc%20pcre:yes&sq=package:chromium&type=cs&all=1

I have tried to tune the regular expressions to include all cases where
the conditional expression is broken around the ":". The result is about
400:1000 (this time excluding v8 and usr/include). Yes, it is a preference,
but clang-format's style is far from unprecedented.

Two more tweaks your regex needs to make:
(1) Exclude third_party.
(2) Count the cases that break after '?' and _not_ around ':', i.e.

a ?
     b : c;

When both of these are done, you have 260 results formatted your way versus
1282 formatted mine*.

To me this is compelling.

1) I don't want to flip-flop. You are the first Chromium developer to

complain about this.

AFAIK based on mast chromium-dev mails, clang-format has not even worked on
Chromium code until comparatively recently, and is not an explicitly
encouraged part of our workflow. Certainly there are developers using it,
but I would be surprised if it was as much as even 20% of Chromium devs,
and most of them not for more than a couple of months.

I would not go so far as to concluded that no one has a positive opinion
about this style, but I don't think it's valid to conclude that "many
others like it" either.

PK

*Here are the actual regexes:

https://code.google.com/p/chromium/codesearch#search/&q=(?m:\?.*\n\%20*\:)%20-file:v8%20-file:usr/include%20-file:third_party%20lang:cc%20pcre:yes&sq=package:chromium&type=cs

vs.

https://code.google.com/p/chromium/codesearch#search/&q=(?m:\?\n{0,1}.*\%20\:$)%20-file:v8%20-file:usr/include%20-file:third_party%20lang:cc%20pcre:yes&sq=package:chromium&type=cs