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

[curdeius]

Summary

This fixes bug 25192.
See https://llvm.org/bugs/show_bug.cgi?id=25192.

[djasper]

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]

• 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]

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]

Ping?

[curdeius]

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

lib/Format/UnwrappedLineFormatter.cpp

tryFitMultipleLinesInOne(unsigned Indent,
     // If we already exceed the column limit, we set 'Limit' to 0. The different
     // tryMerge..() functions can then decide whether to still do merging.
     Limit = TheLine->Last->TotalLength > Limit
                 ? 0
                 : Limit - TheLine->Last->TotalLength;
 
     // FIXME: TheLine->Level != 0 might or might not be the right check to do.
     // If necessary, change to something smarter.
     bool MergeShortFunctions =
         Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All ||
         (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty &&
-         I[1]->First->is(tok::r_brace)) ||
+         I[1]->First->isOneOf(tok::l_brace, tok::r_brace)) ||
         (Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Inline &&
          TheLine->Level != 0);
 
     if (TheLine->Last->is(TT_FunctionLBrace) &&
         TheLine->First != TheLine->Last) {
       return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
     }
     if (TheLine->Last->is(tok::l_brace)) {

unittests/Format/FormatTest.cpp

verifyFormat("class C {\n"
                "};",
                MergeInlineOnly);
   verifyFormat("int f() {\n"
                "  return 42;\n"
                "}",
                MergeInlineOnly);
 }
 
+TEST_F(FormatTest, PullEmptyFunctionDefinitionsIntoSingleLineLinux) {
+  FormatStyle MergeEmptyLinux = getLLVMStyle();
+  MergeEmptyLinux.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty;
+  MergeEmptyLinux.BreakBeforeBraces = FormatStyle::BS_Linux;
+  verifyFormat("class C\n"
+               "{\n"
+               "  int f() {}\n"
+               "};",
+               MergeEmptyLinux);
+  verifyFormat("int f() {}", MergeEmptyLinux);
+}
+
 TEST_F(FormatTest, UnderstandContextOfRecordTypeKeywords) {
   // Elaborate type variable declarations.
   verifyFormat("struct foo a = {bar};\nint n;");
   verifyFormat("class foo a = {bar};\nint n;");
   verifyFormat("union foo a = {bar};\nint n;");
 
   // Elaborate types inside function definitions.
   verifyFormat("struct foo f() {}\nint n;");