[PATCH] Prefer breaking in places other than '(' for default style

[Eric Christopher]

(Beware, somewhat opinionated crap below - it is formatting and indentation
after all.)

On Tue, Oct 22, 2013 at 11:18 PM, Daniel Jasper <djasper at google.com> wrote:

> My thoughts:
> - "more legible" is highly subjective. I for one like clang-format's
> choice as it uses fewer lines and (more importantly) fewer different
> indents. So if I had to manually choose between the two, I would choose the
> former.
>

I suppose, I'd have considered it more legible because it helps avoid what
I like to call the "wall of text" problem (not actually picking on
dblaikie, but it came up yesterday):


         sh_link = SectionIndexMap.lookup(Asm.getContext().getELFSection(
          SecName.substr(sizeof(".ARM.exidx") - 1), ELF::SHT_PROGBITS,
          ELF::SHF_EXECINSTR | ELF::SHF_ALLOC, SectionKind::getText(), 0,
          GroupName));


This sort of thing is what you get with the current indenting scheme
(though to be fair, my patch won't fix this particular one). There's no
real hierarchical look going on here, and the indenting isn't helping
readability. I.e. if we only optimize for "fewer lines and fewer indents"
then we get more and more walls of text - especially when we're looking at
lots of nested calls with their own breaks at '('. :)

- We have explicitly changed this for Google style and we are not changing
> it back. So if this change to submit this change, we need to introduce an
> additional style option (and I have no idea what to call it ;-) ). Maybe
> the best choice would be to pull out the actual penalty into something that
> can be configured per style. E.g.: PenaltyWrapCallAfterParen.
>
>
Totally up for that.


> - There is an alternative to consider. In earlier days of clang-format we
> used hanging identation (we still do with all other binary operators). I.e.
> the snippet you mention would be formatted as:
>
> CharSourceRange LineRange = CharSourceRange::getTokenRange(
>                                 Line.Tokens.front().Tok.getLo(),
>                                 Line.Tokens.back().Tok.getLoc());
>
> Now, arguably, this is preferable as it is more structured than what
> clang-format currently does and at the same time does not need three
> different indentations (which makes it look 'untidy'). That option,
> however, did not fly in Google style and we had to special-case all
> assignment expressions. Again, that is something we could change
> specifically for LLVM style. However, from offline discussions I assume
> that breaking after the opening parenthesis bothers you in more places.
> Just wanted to bring this back onto the table as I know Chandler is a big
> fan :-).
>

It's definitely preferable, though as you surmise, not my preference. I
prefer groupings of things that are easy to see and in the "prefer breaking
after the ( versus breaking after the =" argument the "type and name of the
value being assigned into" is always going to lose to "fewer lines" :)

FWIW I've run this patch across quite a bit of code at this point and it
largely comes out neutral for code size - the heuristic aspects in the rest
of clang format stop it from getting out of control. I think the net across
a bunch of files was about 4 lines. Mostly it just changes where we break,
it takes some special arguments or longish chains of names to actually
increase the indent.

Thanks :)

-eric


>
> Cheers,
> Daniel
>
>
> On Wed, Oct 23, 2013 at 2:46 AM, Eric Christopher <echristo at gmail.com>wrote:
>
>> Bah. The formatting didn't go in the email as I'd expected.
>>
>> Before:
>>
>>
>>     1.  CharSourceRange LineRange = CharSourceRange::getTokenRange(
>>    2.  Line.Tokens.front().Tok.getLo(), Line.Tokens.back().Tok.getLoc());
>>    3.
>>
>>
>> After:
>>
>>
>>    1.  CharSourceRange LineRange =
>>    2.  CharSourceRange::getTokenRange(Line.Tokens.front().Tok.getLo(),
>>    3.  Line.Tokens.back().Tok.getLoc());
>>
>>
>> Whee.
>>
>> -eric
>>
>>
>>
>> On Tue, Oct 22, 2013 at 5:37 PM, Eric Christopher <echristo at gmail.com>wrote:
>>
>>> Hi Daniel,
>>>
>>> Thought I'd send it out and get some discussion if people care and
>>> then I can update the tests and commit if we think it's a good idea.
>>>
>>> It'll change code like in (one of the) failing tests:
>>>
>>> Value of: format(messUp(Code), Style)
>>>   Actual: "CharSourceRange LineRange =\n
>>> CharSourceRange::getTokenRange(Line.Tokens.front().Tok.getLo(),\n
>>>                              Line.Tokens.back().Tok.getLoc());"
>>> Expected: Code.str()
>>> Which is: "CharSourceRange LineRange =
>>> CharSourceRange::getTokenRange(\n    Line.Tokens.front().Tok.getLo(),
>>> Line.Tokens.back().Tok.getLoc());"
>>> [  FAILED  ] FormatTest.BreaksAfterAssignments (5 ms)
>>> [----------] 1 test from FormatTest (5 ms total)
>>>
>>> this:
>>>
>>> <pre style='color:#000000;background:#ffffff;'><html><body
>>> style='color:#000000; background:#ffffff; '><pre>
>>> CharSourceRange LineRange <span style='color:#808030; '>=</span>
>>>       CharSourceRange<span style='color:#800080;
>>> '>::</span>getTokenRange<span style='color:#808030;
>>> '>(</span>Line<span style='color:#808030; '>.</span>Tokens<span
>>> style='color:#808030; '>.</span>front<span style='color:#808030;
>>> '>(</span><span style='color:#808030; '>)</span><span
>>> style='color:#808030; '>.</span>Tok<span style='color:#808030;
>>> '>.</span>getLo<span style='color:#808030; '>(</span><span
>>> style='color:#808030; '>)</span><span style='color:#808030; '>,</span>
>>>                                      Line<span style='color:#808030;
>>> '>.</span>Tokens<span style='color:#808030; '>.</span>back<span
>>> style='color:#808030; '>(</span><span style='color:#808030;
>>> '>)</span><span style='color:#808030; '>.</span>Tok<span
>>> style='color:#808030; '>.</span>getLoc<span style='color:#808030;
>>> '>(</span><span style='color:#808030; '>)</span><span
>>> style='color:#808030; '>)</span><span style='color:#800080; '>;</span>
>>> </pre>
>>>
>>> versus:
>>>
>>> <pre style='color:#000000;background:#ffffff;'><html><body
>>> style='color:#000000; background:#ffffff; '><pre>
>>> CharSourceRange LineRange <span style='color:#808030; '>=</span>
>>> CharSourceRange<span style='color:#800080;
>>> '>::</span>getTokenRange<span style='color:#808030; '>(</span>
>>>         Line<span style='color:#808030; '>.</span>Tokens<span
>>> style='color:#808030; '>.</span>front<span style='color:#808030;
>>> '>(</span><span style='color:#808030; '>)</span><span
>>> style='color:#808030; '>.</span>Tok<span style='color:#808030;
>>> '>.</span>getLo<span style='color:#808030; '>(</span><span
>>> style='color:#808030; '>)</span><span style='color:#808030; '>,</span>
>>> Line<span style='color:#808030; '>.</span>Tokens<span
>>> style='color:#808030; '>.</span>back<span style='color:#808030;
>>> '>(</span><span style='color:#808030; '>)</span><span
>>> style='color:#808030; '>.</span>Tok<span style='color:#808030;
>>> '>.</span>getLoc<span style='color:#808030; '>(</span><span
>>> style='color:#808030; '>)</span><span style='color:#808030;
>>> '>)</span><span style='color:#800080; '>;</span>
>>> </pre>
>>>
>>> which while the former is more lines I think it is a much more legible
>>> general formatting style.
>>>
>>> Thoughts?
>>>
>>> -eric
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131023/09fde21d/attachment.html>