Fix bug in clang-format while merging short function (PR19461)

[dinesh.d]

Summary

if Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All, clang-format tries to join short functions and prints in single line.
This code was not taking care of cases where there is something in between function header and body e.g. attached test cases where
there are preprocessor directives in between function header and body.

Attached patch fixes this issue by checking if we have both header and body and nothing between them while trying to merge functions.

Please let me know if this is ok.

[dinesh.d]

Hi Klimek,

Thanks for review.

With this change, I am trying to figure out whether there is a "gap" in the line.
If this gap is just white spaces, that is not an issue and we can still merge
function. But if there are other constructs such as preprocessor directives, we
should not merge this function to single line.

During debugging, I have observed that if there are other constructs, block before
and after the construct (inclusive) are handled separately [Basically, I am trying
to say is I found TheLine->MustBeDeclaration to be false for 'after' block in such
cases]. I assumed, just checking for this case is good enough, as asserted from
test cases too.

If you think this is not correct, I will look in to code and try to find better fix.

Regards

[dinesh.d]

• updated comment for typos

Hi Klimek,

Thanks for review.

With this change, I am trying to figure out whether there is a "gap" in the line.
If this gap is just white spaces, that is not an issue and we can still merge
function. But if there are other constructs such as preprocessor directives, we
should not merge this function to single line.

During debugging, I have observed that if there are other constructs, block before
and after the construct (inclusive) are handled separately [Basically, I am trying
to say is I found TheLine->MustBeDeclaration to be false for 'after' block in such
cases]. I assumed, just checking for this case is good enough, as asserted from
test cases too.

If you think this is not correct, I will look in to code and try to find better fix.

Regards

[dinesh.d]

Update patch to check First->MustBreakBefore of every line before joining them.

I have updated patch as per you comment. I am checking for MustBreakBefore for
only first token of every line. I was just wondering if AnnotatedLine can have
MustBreakBefore flag set for some internal token and how we honour it while
formatting the line.

Can an AnnotatedLine have MustBreakBefore flag set for internal tokens as well?
If so, then we have to check this flag for all tokens and their children. Will
it be ok to add a 'IsJoinable' flag in AnnotatedLine and set it accordingly
while constructing AnnotatedLine.

[djasper]

lib/Format/Format.cpp

641

Why would it be a problem if we need to wrap before the first of the lines we are going to join. I think the test case passes for the wrong reason. Of course MustBreakBefore is set for the first token of:

"void foo().."

But that is not the point. We are actually interested at whether there is any token other than the first where MustBreakBefore is set (the "{" in the case of the test).

664

How about we put this check into "nextTwoLinesFitInto"?

[dinesh.d]

The problem is due to code 'I[1]->First->Type == TT_FunctionLBrace' [Format.cpp:562]
where we assume that I[0] contains something which we can join with rest of the lines.
I have experimented with logic to check flags for all token in lines being merged
except first token for MustBreakBefore flags and they cause other side effects while
solving this problem.

I have re-placed check for First->MustBreakBefore from 'tryMergeSimpleBlock()' to
'tryFitMultipleLinesInOne()' so it will only take effect for above case. I have updated
test cases to check for even more conditions.

[djasper]

Ah right, this is for styles that break before the braces (I got confused).

Two small comments, otherwise this seems like a strict improvement now.

lib/Format/Format.cpp

562

How about moving the MustBreakBefore check to line 535, i.e.:

if (I + 1 == E || I[1]->Type == LT_Invalid || I[1]->MustBreakBefore)
  return 0;

I think it is trivially correct and much easier to see that I[1] is guaranteed to exist.

unittests/Format/FormatTest.cpp

7365

Can you split these into smaller verifyFormat() calls that test each case (or smaller group of cases)? It has proven to be much easier to figure out what is going wrong if the tests are as small as possible. Otherwise you get a large diff in the test output and can't easily see what is going on. Same below.

[dinesh.d]

Updated as per review comments.

I have moved 'I[1]->First->MustBreakBefore' check to line 535. I agree that this is good as there are other check for I[1] but this check is actually required in context at for block of lines 554:570. For lines between 535:554, it is redundant check as this condition is already being checked in tryMergeSimpleBlock().

[dinesh.d]

gentle ping

[djasper]

A few indentations aside, this looks good.

unittests/Format/FormatTest.cpp

7477

The indentation here is off. Maybe use clang-format ;-).

7487

Same here.

7628

Same here.

7638

Same here.

[dinesh.d]

updated patch to fix formating

[dinesh.d]

submitted (r207958)

Thanks for review and support

lib/Format/Format.cpp

     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;
 
-    if (I + 1 == E || I[1]->Type == LT_Invalid)
+    if (I + 1 == E || I[1]->Type == LT_Invalid || I[1]->First->MustBreakBefore)
       return 0;
 
     // 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_Inline &&
          TheLine->Level != 0);

     if (I[1]->First->Type == TT_FunctionLBrace &&
         Style.BreakBeforeBraces != FormatStyle::BS_Attach) {
       // Check for Limit <= 2 to account for the " {".
       if (Limit <= 2 || (Style.ColumnLimit == 0 && containsMustBreak(TheLine)))
         return 0;
       Limit -= 2;
 
       unsigned MergedLines = 0;
       if (MergeShortFunctions) {
         MergedLines = tryMergeSimpleBlock(I + 1, E, Limit);
         // If we managed to merge the block, count the function header, which is
         // on a separate line.
         if (MergedLines > 0)
           ++MergedLines;
       }
       return MergedLines;
     }

   unsigned
   tryMergeSimpleBlock(SmallVectorImpl<AnnotatedLine *>::const_iterator I,
                       SmallVectorImpl<AnnotatedLine *>::const_iterator E,
                       unsigned Limit) {
     // First, check that the current line allows merging. This is the case if
     // we're not in a control flow statement and the last token is an opening
     // brace.
     AnnotatedLine &Line = **I;
     if (Line.First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_do, tok::r_brace,
                             tok::kw_else, tok::kw_try, tok::kw_catch,
                             tok::kw_for, tok::kw_case,
                             // This gets rid of all ObjC @ keywords and methods.
                             tok::at, tok::minus, tok::plus))
       return 0;
 
     FormatToken *Tok = I[1]->First;
     if (Tok->is(tok::r_brace) && !Tok->MustBreakBefore &&
         (Tok->getNextNonComment() == NULL ||
          Tok->getNextNonComment()->is(tok::semi))) {
       // We merge empty blocks even if the line exceeds the column limit.
       Tok->SpacesRequiredBefore = 0;
       Tok->CanBreakBefore = true;
       return 1;
     } else if (Limit != 0 && Line.First->isNot(tok::kw_namespace)) {
       // Check that we still have three lines and they fit into the limit.
       if (I + 2 == E || I[2]->Type == LT_Invalid)
         return 0;
       Limit = limitConsideringMacros(I + 2, E, Limit);
 
       if (!nextTwoLinesFitInto(I, Limit))
         return 0;
 
       // Second, check that the next line does not contain any braces - if it
       // does, readability declines when putting it into a single line.
-      if (I[1]->Last->Type == TT_LineComment || Tok->MustBreakBefore)
+      if (I[1]->Last->Type == TT_LineComment)
         return 0;
       do {
         if (Tok->isOneOf(tok::l_brace, tok::r_brace))
           return 0;
         Tok = Tok->Next;
       } while (Tok != NULL);
 
       // Last, check that the third line contains a single closing brace.
       Tok = I[2]->First;
-      if (Tok->getNextNonComment() != NULL || Tok->isNot(tok::r_brace) ||
-          Tok->MustBreakBefore)
+      if (Tok->getNextNonComment() != NULL || Tok->isNot(tok::r_brace))
         return 0;
 
       return 2;
     }
     return 0;
   }
 
   /// Returns the modified column limit for \p I if it is inside a macro and
   /// needs a trailing '\'.
   unsigned
   limitConsideringMacros(SmallVectorImpl<AnnotatedLine *>::const_iterator I,
                          SmallVectorImpl<AnnotatedLine *>::const_iterator E,
                          unsigned Limit) {
     if (I[0]->InPPDirective && I + 1 != E &&
         !I[1]->First->HasUnescapedNewline && !I[1]->First->is(tok::eof)) {
       return Limit < 2 ? 0 : Limit - 2;
     }
     return Limit;
   }
 
   bool nextTwoLinesFitInto(SmallVectorImpl<AnnotatedLine *>::const_iterator I,
                            unsigned Limit) {
+    if (I[1]->First->MustBreakBefore || I[2]->First->MustBreakBefore)
+      return false;
     return 1 + I[1]->Last->TotalLength + 1 + I[2]->Last->TotalLength <= Limit;
   }
 
   bool containsMustBreak(const AnnotatedLine *Line) {
     for (const FormatToken *Tok = Line->First; Tok; Tok = Tok->Next) {
       if (Tok->MustBreakBefore)
         return true;
     }

unittests/Format/FormatTest.cpp

   verifyFormat("namespace a {\n"
                "class A {\n"
                "  void f()\n"
                "  {\n"
                "    if (true) {\n"
                "      a();\n"
                "      b();\n"
                "    }\n"
                "  }\n"
                "  void g() { return; }\n"
                "}\n"
                "}",
                BreakBeforeBrace);
+
+  verifyFormat("#ifdef _DEBUG\n"
+               "int foo(int i = 0)\n"
+               "#else\n"
+               "int foo(int i = 5)\n"
+               "#endif\n"
+               "{\n"
+               "  return i;\n"
+               "}",
+               BreakBeforeBrace);
+
+  verifyFormat("void foo() {}\n"
+               "void bar()\n"
+               "#ifdef _DEBUG\n"
+               "{\n"
+               "  foo();\n"
+               "}\n"
+               "#else\n"
+               "{\n"
+               "}\n"
+               "#endif",
+               BreakBeforeBrace);
+
+  verifyFormat("void foobar() { int i = 5; }\n"
+               "#ifdef _DEBUG\n"
+               "void bar() {}\n"
+               "#else\n"
+               "void bar() { foobar(); }\n"
+               "#endif",
+               BreakBeforeBrace);
 }
 
 TEST_F(FormatTest, AllmanBraceBreaking) {
   FormatStyle BreakBeforeBrace = getLLVMStyle();
   BreakBeforeBrace.BreakBeforeBraces = FormatStyle::BS_Allman;
   verifyFormat("namespace a\n"
                "{\n"
                "class A\n"

                "  break;\n"
                "  default:\n"
                "    break;\n"
                "  }\n"
                "}\n",
                BreakBeforeBrace);
 
   verifyFormat("enum X\n"
                "{\n"
                "  Y = 0,\n"
                "}\n",
                BreakBeforeBrace);
 
   verifyFormat("@interface BSApplicationController ()\n"
                "{\n"
                "@private\n"
                "  id _extraIvar;\n"
                "}\n"
                "@end\n",
                BreakBeforeBrace);
 
+  verifyFormat("#ifdef _DEBUG\n"
+               "int foo(int i = 0)\n"
+               "#else\n"
+               "int foo(int i = 5)\n"
+               "#endif\n"
+               "{\n"
+               "  return i;\n"
+               "}",
+               BreakBeforeBrace);
+
+  verifyFormat("void foo() {}\n"
+               "void bar()\n"
+               "#ifdef _DEBUG\n"
+               "{\n"
+               "  foo();\n"
+               "}\n"
+               "#else\n"
+               "{\n"
+               "}\n"
+               "#endif",
+               BreakBeforeBrace);
+
+  verifyFormat("void foobar() { int i = 5; }\n"
+               "#ifdef _DEBUG\n"
+               "void bar() {}\n"
+               "#else\n"
+               "void bar() { foobar(); }\n"
+               "#endif",
+               BreakBeforeBrace);
+
   BreakBeforeBrace.ColumnLimit = 19;
   verifyFormat("void f() { int i; }", BreakBeforeBrace);
   BreakBeforeBrace.ColumnLimit = 18;
   verifyFormat("void f()\n"
                "{\n"
                "  int i;\n"
                "}",
                BreakBeforeBrace);

                "      break;\n"
                "    case 1:\n"
                "      {\n"
                "        break;\n"
                "      }\n"
                "    case 2:\n"
                "      {\n"
                "      }\n"
                "      break;\n"
                "    default:\n"
                "      break;\n"
                "    }\n"
                "}\n",
                GNUBraceStyle);
 
   verifyFormat("enum X\n"
                "{\n"
                "  Y = 0,\n"
                "}\n",
                GNUBraceStyle);
 
   verifyFormat("@interface BSApplicationController ()\n"
                "{\n"
                "@private\n"
                "  id _extraIvar;\n"
                "}\n"
                "@end\n",
                GNUBraceStyle);
+
+  verifyFormat("#ifdef _DEBUG\n"
+               "int foo(int i = 0)\n"
+               "#else\n"
+               "int foo(int i = 5)\n"
+               "#endif\n"
+               "{\n"
+               "  return i;\n"
+               "}",
+               GNUBraceStyle);
+
+  verifyFormat("void foo() {}\n"
+               "void bar()\n"
+               "#ifdef _DEBUG\n"
+               "{\n"
+               "  foo();\n"
+               "}\n"
+               "#else\n"
+               "{\n"
+               "}\n"
+               "#endif",
+               GNUBraceStyle);
+
+  verifyFormat("void foobar() { int i = 5; }\n"
+               "#ifdef _DEBUG\n"
+               "void bar() {}\n"
+               "#else\n"
+               "void bar() { foobar(); }\n"
+               "#endif",
+               GNUBraceStyle);
 }
 TEST_F(FormatTest, CatchExceptionReferenceBinding) {
   verifyFormat("void f() {\n"
                "  try {\n"
                "  }\n"
                "  catch (const Exception &e) {\n"
                "  }\n"
                "}\n",