This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] AllowShortFunctionsOnASingleLine: true/Empty didn't work with BreakBeforeBraces: Linux/Allman.
AbandonedPublic

Authored by curdeius on Oct 16 2015, 7:28 AM.

Details

Reviewers
djasper
Summary

Diff Detail

Event Timeline

curdeius updated this revision to Diff 37585.Oct 16 2015, 7:28 AM
curdeius retitled this revision from to [clang-format] AllowShortFunctionsOnASingleLine: true didn't work with BreakBeforeBraces: Linux/Allman..
curdeius updated this object.
curdeius added a reviewer: djasper.
curdeius added a subscriber: cfe-commits.
curdeius retitled this revision from [clang-format] AllowShortFunctionsOnASingleLine: true didn't work with BreakBeforeBraces: Linux/Allman. to [clang-format] AllowShortFunctionsOnASingleLine: true/Empty didn't work with BreakBeforeBraces: Linux/Allman..Oct 16 2015, 7:28 AM
curdeius set the repository for this revision to rL LLVM.
djasper added inline comments.Oct 18 2015, 12:34 AM
lib/Format/UnwrappedLineFormatter.cpp
193

From a first look, this doesn't make sense. We specifically check that the next line is a closing brace to ensure that this is an empty function. Now, this seems to work with the existing tests, but because of very intricate reasons:

  • If we are not wrapping with Allman/Linux style, the first token of I[1] is unlikely to be an open brace. It can actually be one: void f() { { DoSomething(); } }, but in this case there are too many unwrapped lines to merge anything.
  • There aren't any other tests with Allman/Linux style *and* AllowShortFunctionsOnASingleLine set to "Empty". If there were, they'd show, that this code now also merges non-empty functinos.

Instead, pull out an additional variable:

bool EmptyFunction = (Style.BraceWrapping.AfterFunction ? I[2] : I[1])->First->is(tok::r_brace);

and then use that.

Please also add a test showing that non-empty functions do not get merged.

unittests/Format/FormatTest.cpp
6523

Just extend PullInlineFunctionDefinitionsIntoSingleLine instead.

curdeius updated this revision to Diff 37832.Oct 20 2015, 1:30 AM
  • AllowShortFunctionsOnASingleLine: true didn't work with BreakBeforeBraces: Linux/Allman.
  • Add test checking that non-empty functions in styles with BraceWrapping.AfterFunction = true don't get merged into one line. Fix the merge condition.
curdeius marked 2 inline comments as done.Oct 20 2015, 1:34 AM

Applied comments.

lib/Format/UnwrappedLineFormatter.cpp
190

IMO, (I + 2 != E) is a bit awkward yet necessary here, but I don't see a better way to check if we have more than two tokens.

curdeius abandoned this revision.Sep 20 2017, 5:44 AM

This was fixed by https://reviews.llvm.org/rL312904 and other commits.