Page MenuHomePhabricator

clang-format: Add an additional value to AlignAfterOpenBracket: AlwaysBreak.
ClosedPublic

Authored by djasper on Oct 26 2015, 9:17 PM.

Details

Summary

If this option is set, clang-format will always insert a line wrap, e.g.
before the first parameter of a function call unless all parameters fit
on the same line. This obviates the need to make a decision on the
alignment itself.

Use this style for Google's JavaScript style and add some minor tweaks
to correctly handle nested blocks etc. with it. Don't use this option
for for/while loops.

Diff Detail

Repository
rL LLVM

Event Timeline

djasper updated this revision to Diff 38496.Oct 26 2015, 9:17 PM
djasper retitled this revision from to clang-format: Add an additional value to AlignAfterOpenBracket: AlwaysBreak..
djasper updated this object.
djasper added a reviewer: klimek.
djasper added a subscriber: cfe-commits.
djasper removed a subscriber: klimek.
mprobst edited edge metadata.Oct 27 2015, 2:54 AM

Nice, great to see this.

lib/Format/ContinuationIndenter.cpp
330 ↗(On Diff #38496)

Nice.

Maybe add a comment along the lines off // In "AlwaysBreak" mode, enforce wrapping directly after the parenthesis by disallowing any further line breaks if there is no line break after the opening parenthesis or so?

882 ↗(On Diff #38496)

Maybe add a comment on how this works? It's not clear to me. Is this change (with the assignment below) to allow breaks in block-style literals?

983 ↗(On Diff #38496)

Nit: add a comment on what the parameter is? /*NoLineBreak*/ false? And maybe explain why code below needs this to be false?

984 ↗(On Diff #38496)

Nit: leftover comment?

lib/Format/TokenAnnotator.cpp
393 ↗(On Diff #38496)

I don't understand why this changes how we count block parameters - is this an unrelated cleanup?

unittests/Format/FormatTestJS.cpp
296 ↗(On Diff #38496)

nit: typo in SomeFunction

325 ↗(On Diff #38496)

This formatting is ok, I guess (and hopefully not too common in the first place), but I find it somewhat surprising that this is a side effect of this change. Can you explain?

djasper updated this revision to Diff 38518.Oct 27 2015, 3:13 AM
djasper marked 5 inline comments as done.
djasper edited edge metadata.

Addressed review comments.

lib/Format/ContinuationIndenter.cpp
882 ↗(On Diff #38496)

Yes. Added a comment.

lib/Format/TokenAnnotator.cpp
393 ↗(On Diff #38496)

The other changes uncovered that we weren't properly counting nested Java blocks.

klimek added inline comments.Oct 27 2015, 3:21 AM
lib/Format/ContinuationIndenter.cpp
334 ↗(On Diff #38518)

Is State.Column > getNewLineColumn(State) used to check for the line break?

djasper added inline comments.Oct 27 2015, 3:36 AM
lib/Format/ContinuationIndenter.cpp
334 ↗(On Diff #38518)

No. It is so that we don't enforce a break when it is not conserving columns, e.g. we'd still do:

f(aaaa,
  bbbb);
klimek added inline comments.Oct 27 2015, 3:42 AM
lib/Format/ContinuationIndenter.cpp
334 ↗(On Diff #38518)

Ah, please add that to the comment then (it is surprising to me that this is wanted in always-break mode).

djasper marked an inline comment as done.Oct 27 2015, 4:08 AM
djasper updated this revision to Diff 38524.Oct 27 2015, 4:09 AM

Extended comment.

mprobst accepted this revision.Oct 27 2015, 5:37 AM
mprobst edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Oct 27 2015, 5:37 AM
This revision was automatically updated to reflect the committed changes.
chfast added a subscriber: chfast.Dec 8 2017, 7:13 AM
chfast added inline comments.
cfe/trunk/docs/ClangFormatStyleOptions.rst
173

I think this example is not achievable. clang-format seems to prefer putting all arguments on the next line if they all fit there. So this one will be formatted the same as the example for AlwaysBreak.