On Wed, Oct 23, 2013 at 10:42 PM, Eric Christopher <[email protected]>wrote:
> (Beware, somewhat opinionated crap below - it is formatting and > indentation after all.) > > On Tue, Oct 22, 2013 at 11:18 PM, Daniel Jasper <[email protected]>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)); > a) how would you want to layout this b) why is this bad? to me it's actually very quick to read... (I'd probably lay it out manually exactly like that) > 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 <[email protected]>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 <[email protected]>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 >>>> >>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
