[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

198–199

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

6538

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,
     unsigned Limit =
         Style.ColumnLimit == 0 ? UINT_MAX : Style.ColumnLimit - 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;
 
+    bool EmptyFunction =
+        (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty) &&
+        (((Style.BraceWrapping.AfterFunction && (I + 2 != E)) ? I[2] : I[1])
+             ->First->is(tok::r_brace));
     // FIXME: TheLine->Level != 0 might or might not be the right check to do.
     // If necessary, change to something smarter.
+    bool InlineFunction =
+        (Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Inline) &&
+        (TheLine->Level != 0);
     bool MergeShortFunctions =
         Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All ||
-        (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty &&
-         I[1]->First->is(tok::r_brace)) ||
-        (Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Inline &&
-         TheLine->Level != 0);
+        EmptyFunction || InlineFunction;
 
     if (TheLine->Last->is(TT_FunctionLBrace) &&
         TheLine->First != TheLine->Last) {
       return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
     }
     if (TheLine->Last->is(tok::l_brace)) {
       return !Style.BraceWrapping.AfterFunction
                  ? tryMergeSimpleBlock(I, E, Limit)

unittests/Format/FormatTest.cpp

TEST_F(FormatTest, PullInlineFunctionDefinitionsIntoSingleLine) {
   verifyFormat("class C {\n"
                "  int f() { return 42; }\n"
                "};",
                MergeInlineOnly);
   verifyFormat("int f() {\n"
                "  return 42;\n"
                "}",
                MergeInlineOnly);
+
+  FormatStyle MergeEmptyCustom = getLLVMStyle();
+  MergeEmptyCustom.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty;
+  MergeEmptyCustom.BreakBeforeBraces = FormatStyle::BS_Custom;
+  MergeEmptyCustom.BraceWrapping.AfterFunction = true;
+  verifyFormat("class C {\n"
+               "  int f() {}\n"
+               "};",
+               MergeEmptyCustom);
+  verifyFormat("int f() {}", MergeEmptyCustom);
+  verifyFormat("int f()\n"
+               "{\n"
+               "  return 0;\n"
+               "}",
+               MergeEmptyCustom);
 }
 
 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;");
   verifyFormat("class foo f() {}\nint n;");