[cfe-dev] clang-format chromium ternary operator

[Daniel Jasper]

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;

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

I know that a lot of this boils down to personal preference and what
patterns one is used to, but I find the second version significantly more
structured. The semi-objective reasons I can come up with are:

a) The alignment and close proximity of ? and : aid in understanding
that this is a conditional expression and clearly separate / show the
boundaries of the three operands.

b) A conditional expression is a branch and branches in code are
usually vertical.

On Tue, Aug 13, 2013 at 12:08 AM, Peter Kasting <pkasting at chromium.org>wrote:

> On Mon, Aug 12, 2013 at 2:40 PM, Nico Weber <thakis at chromium.org> wrote:
>
>> 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.
>>
>
> "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:%5C?.*%5Cn%5C%20*%5C:)%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:%5C?%5Cn%7B0,1%7D.*%5C%20%5C:$)%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.


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

I think this is always a balancing act between accepting slightly different
(but legal and readable style) and aiming for perfect consistency. IMO,
both Chromium's and Google's style guides are quite elaborate and the
wiggle room they leave should not be taken away completely by many implicit
rules based on the consistency-argument. For me, conditional expressions
are right on the edge. I could probably read code that alternates between
?/: on the new/old line just fine. On the other hand, I find the variety
and inconsistency in indentation that either of the two links above show
quite distracting.

I don't think escalating to chromium-dev is a good idea. That seems like a
waste of a lot of people's time on a corner issue (with only a few thousand
instances in the codebase). Why am I not just making the change? Several
reasons:
1) I don't want to flip-flop. You are the first Chromium developer to
complain about this. Thus, there seem to be many others that are ok with
this or even like it. Now, as I said, I don't know who makes the decisions
for Chromium, but I'd like to have a final decision before changing
anything.
2) To me, this seems to make formatting strictly worse. As I am not working
on the Chromium codebase, that does not count much and will certainly not
influence the final solution, but it somehow adds to my reservations.
3) It is additional implementation effort (although patches are always
welcome :-) ).
4) It is additional maintenance effort as we'd need to create an additional
style option and ensuring that the combinatorial explosion of style options
always do the right thing is difficult (e.g. how to indent conditional
expressions that are parameters to ObjC functions..).

Cheers,
Daniel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20130813/a81a55f3/attachment.html>