[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

489

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.

2378

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

489

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

489

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.

2378

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

489

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.
}
}

2378

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

489

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

489

Cool! Thanks!

[krasimir]

@djasper: ping

[djasper]

Looks good.

lib/Format/UnwrappedLineParser.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LIB_FORMAT_UNWRAPPEDLINEPARSER_H
 #define LLVM_CLANG_LIB_FORMAT_UNWRAPPEDLINEPARSER_H
 
 #include "FormatToken.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Format/Format.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/Support/Regex.h"
 #include <list>
 #include <stack>
 
 namespace clang {
 namespace format {
 
 struct UnwrappedLineNode;
private:
   void parseObjCInterfaceOrImplementation();
   void parseObjCProtocol();
   void parseJavaScriptEs6ImportExport();
   bool tryToParseLambda();
   bool tryToParseLambdaIntroducer();
   void tryToParseJSFunction();
   void addUnwrappedLine();
   bool eof() const;
-  void nextToken();
+  void nextToken(llvm::Optional<unsigned> InitialLevel = None);
   const FormatToken *getPreviousToken();
-  void readToken();
+  void readToken(llvm::Optional<unsigned> InitialLevel = None);
 
   // Decides which comment tokens should be added to the current line and which
   // should be added as comments before the next token.
   //
   // Comments specifies the sequence of comment tokens to analyze. They get
   // either pushed to the current line or added to the comments before the next
   // token.
   //

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;
   }
 
-  nextToken(); // Munch the closing brace.
+  nextToken(InitialLevel); // 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 (isOnNewLine(**I) && JustComments && !(*I)->ContinuesLineCommentSection)
       addUnwrappedLine();
     pushToken(*I);
   }
   if (NewlineBeforeNext && JustComments)
     addUnwrappedLine();
   CommentsBeforeNextToken.clear();
 }
 
-void UnwrappedLineParser::nextToken() {
+void UnwrappedLineParser::nextToken(llvm::Optional<unsigned> InitialLevel) {
   if (eof())
     return;
   flushComments(isOnNewLine(*FormatTok));
   pushToken(FormatTok);
   if (Style.Language != FormatStyle::LK_JavaScript)
-    readToken();
+    readToken(InitialLevel);
   else
     readTokenWithJavaScriptASI();
 }
 
 const FormatToken *UnwrappedLineParser::getPreviousToken() {
   // FIXME: This is a dirty way to access the previous token. Find a better
   // solution.
   if (!Line || Line->Tokens.empty())
for (unsigned i = 0, e = Comments.size(); i < e; ++i) {
     if (ShouldPushCommentsInCurrentLine) {
       pushToken(FormatTok);
     } else {
       CommentsBeforeNextToken.push_back(FormatTok);
     }
   }
 }
 
-void UnwrappedLineParser::readToken() {
+void UnwrappedLineParser::readToken(llvm::Optional<unsigned> InitialLevel) {
   SmallVector<FormatToken *, 1> Comments;
   do {
     FormatTok = Tokens->getNextToken();
     assert(FormatTok);
     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);
+      if (InitialLevel)
+        Line->Level = *InitialLevel;
       // 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 ||

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"
             " */",