[clang-format] Fix comment levels between '}' and PPDirective

[krasimir]

Summary

This fixes a regression exposed by r307795 in which the level of a comment line
between '}' and a preprocessor directive is incorrectly set as the level of the
line before the '}'. In effect, this:

int f(int i) {
  int j = i;
  return i + j;
}
// comment

#ifdef A
#endif

was formatted as:

int f(int i) {
  int j = i;
  return i + j;
}
  // comment

#ifdef A
#endif

[djasper]

lib/Format/UnwrappedLineParser.cpp

491

What happens if you instead change the Line->Level = InitialLevel; statement from below to before this line? That seems like the more intuitively correct fix.

2379

LLVM style requires a line break.

unittests/Format/FormatTestComments.cpp

848

Generally, mess up the code in some way to ensure that it is actually being formatted.

[krasimir]

• Fix formatting

[krasimir]

lib/Format/UnwrappedLineParser.cpp

491

This doesn't work since comments before the right brace haven't been emitted yet and would get the wrong level.

unittests/Format/FormatTestComments.cpp

848

Messing up doesn't work in this case, because we rely on the original columns of the comment and the previous line. That's why I added a bunch of tests.

[krasimir]

• Remove TODO test case

[djasper]

lib/Format/UnwrappedLineParser.cpp

491

So that means this seems to be the interesting case:

void f() {
  DoSomething();
  // This was a fun function.
}
// Cool macro:
#define A a

Now, both comments are basically read when we are reading the "}", but they should have different indentation levels. I have another suggestion, see below.

2379

What happens if we always set the Level to 0 here?

unittests/Format/FormatTestComments.cpp

848

I meant *manually* mess up. So add spaces here and there.

[krasimir]

lib/Format/UnwrappedLineParser.cpp

491

Here is another breaking test in case we change Line->Level = InitialLevel to above this line:

switch (x) {
default: {
  // Do nothing.
}
}

gets reformatted as:

switch (x) {
default: {
// Do nothing.
}
}

2379

If we always set the Level to 0 here, then

void  f() {
  int i;
  /* comment */

#define A
}

gets indented as

void  f() {
  int i;
/* comment */

#define A
}

[krasimir]

• Manually messed up tests

[djasper]

lib/Format/UnwrappedLineParser.cpp

491

I think we can fix all of these cases by doing:

flushComments(isOnNewLine(*FormatTok));
Line->Level = InitialLevel;
nextToken(); // Munch the closing brace.

(so add the first two lines here, remove Line->Level = InitialLevel; below.

[krasimir]

• Adapt the fix from comment suggestion

lib/Format/UnwrappedLineParser.cpp

491

Cool! Thanks!

[krasimir]

@djasper: ping

[djasper]

Looks good.

lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
 
   if (MacroBlock ? !FormatTok->is(TT_MacroBlockEnd)
                  : !FormatTok->is(tok::r_brace)) {
     Line->Level = InitialLevel;
     FormatTok->BlockKind = BK_Block;
     return;
   }
 
+  flushComments(isOnNewLine(*FormatTok));
+  Line->Level = InitialLevel;
   nextToken(); // Munch the closing brace.
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
 
   if (MunchSemi && FormatTok->Tok.is(tok::semi))
     nextToken();
-  Line->Level = InitialLevel;
   Line->MatchingOpeningBlockLineIndex = OpeningLineIndex;
   if (OpeningLineIndex != UnwrappedLine::kInvalidIndex) {
     // Update the opening line to add the forward reference as well
     (*CurrentLines)[OpeningLineIndex].MatchingOpeningBlockLineIndex =
             CurrentLines->size() - 1;
   }
 }
 
do {
     while (!Line->InPPDirective && FormatTok->Tok.is(tok::hash) &&
            (FormatTok->HasUnescapedNewline || FormatTok->IsFirst)) {
       distributeComments(Comments, FormatTok);
       Comments.clear();
       // If there is an unfinished unwrapped line, we flush the preprocessor
       // directives only after that unwrapped line was finished later.
       bool SwitchToPreprocessorLines = !Line->Tokens.empty();
       ScopedLineState BlockState(*this, SwitchToPreprocessorLines);
       // Comments stored before the preprocessor directive need to be output
       // before the preprocessor directive, at the same level as the
       // preprocessor directive, as we consider them to apply to the directive.
       flushComments(isOnNewLine(*FormatTok));
       parsePPDirective();
     }
     while (FormatTok->Type == TT_ConflictStart ||
            FormatTok->Type == TT_ConflictEnd ||
            FormatTok->Type == TT_ConflictAlternative) {

unittests/Format/FormatTestComments.cpp

EXPECT_EQ("void f() {\n"
             format("void f() {\n"
                    "  int i;\n"
                    "  /* comment */\n"
                    "\n"
                    "#ifdef A\n"
                    "  int j;\n"
                    "}"));
 
+  EXPECT_EQ("int f(int i) {\n"
+            "  if (true) {\n"
+            "    ++i;\n"
+            "  }\n"
+            "  // comment\n"
+            "#ifdef A\n"
+            "  int j;\n"
+            "#endif\n"
+            "}",
+            format("int f(int i) {\n"
+                   "  if (true) {\n"
+                   "    ++i;\n"
+                   "  }\n"
+                   "  // comment\n"
+                   "#ifdef A\n"
+                   "int j;\n"
+                   "#endif\n"
+                   "}"));
+
   // Keep the current level if there is an empty line between the comment and
   // the preprocessor directive.
   EXPECT_EQ("void f() {\n"
             "  int i;\n"
             "  /* comment */\n"
             "\n"
             "#ifdef A\n"
             "  int j;\n"
             "}",
             format("void f() {\n"
                    "  int i;\n"
                    "/* comment */\n"
                    "\n"
                    "#ifdef A\n"
                    "  int j;\n"
                    "}"));
 
+  EXPECT_EQ("void f() {\n"
+            "  int i;\n"
+            "  return i;\n"
+            "}\n"
+            "// comment\n"
+            "\n"
+            "#ifdef A\n"
+            "int i;\n"
+            "#endif // A",
+            format("void f() {\n"
+                   "   int i;\n"
+                   "  return i;\n"
+                   "}\n"
+                   "// comment\n"
+                   "\n"
+                   "#ifdef A\n"
+                   "int i;\n"
+                   "#endif // A"));
+
+  EXPECT_EQ("int f(int i) {\n"
+            "  if (true) {\n"
+            "    ++i;\n"
+            "  }\n"
+            "  // comment\n"
+            "\n"
+            "#ifdef A\n"
+            "  int j;\n"
+            "#endif\n"
+            "}",
+            format("int f(int i) {\n"
+                   "   if (true) {\n"
+                   "    ++i;\n"
+                   "  }\n"
+                   "  // comment\n"
+                   "\n"
+                   "#ifdef A\n"
+                   "  int j;\n"
+                   "#endif\n"
+                   "}"));
+
   // Align with the preprocessor directive if the comment was originally aligned
   // with the preprocessor directive.
   EXPECT_EQ("void f() {\n"
             "  int i;\n"
             "/* comment */\n"
             "#ifdef A\n"
             "  int j;\n"
             "}",
             format("void f() {\n"
                    "  int i;\n"
                    "/* comment */\n"
                    "#ifdef A\n"
                    "  int j;\n"
                    "}"));
+
+  EXPECT_EQ("int f(int i) {\n"
+            "  if (true) {\n"
+            "    ++i;\n"
+            "  }\n"
+            "// comment\n"
+            "#ifdef A\n"
+            "  int j;\n"
+            "#endif\n"
+            "}",
+            format("int f(int i) {\n"
+                   "   if (true) {\n"
+                   "    ++i;\n"
+                   "  }\n"
+                   "// comment\n"
+                   "#ifdef A\n"
+                   "  int j;\n"
+                   "#endif\n"
+                   "}"));
 }
 
 TEST_F(FormatTestComments, SplitsLongLinesInComments) {
   EXPECT_EQ("/* This is a long\n"
             " * comment that\n"
             " * doesn't\n"
             " * fit on one line.\n"
             " */",