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

[krasimir]

Summary

This fixes a regression exposed by r307795 and rL308725 in which the level of a
comment line between '} else {' and a preprocessor directive is incorrectly set
as the level of the '} else {' line. For example, this :

int f(int i) {
  if (i) {
    ++i;
  } else {
    // comment
#ifdef A
    --i;
#endif
  }
}

was formatted as:

int f(int i) {
  if (i) {
    ++i;
  } else {
  // comment
#ifdef A
    --i;
#endif
  }
}

[krasimir]

• Make the closing brace use the same thing

[krasimir]

• Switch to relative levels

[krasimir]

• Remove unused import

[klimek]

lg

lib/Format/UnwrappedLineParser.h

126 ↗

(On Diff #107901)

s/this token/the current token/
"This token" can be interpreted as the one nextToken reads otherwise.

[krasimir]

• Address review comments

cfe/trunk/lib/Format/TokenAnnotator.cpp

return Left.isOneOf(tok::comma, tok::coloncolon, tok::semi, tok::l_brace,
                        tok::colon, tok::l_square, tok::at) ||
          (Left.is(tok::r_paren) &&
           Right.isOneOf(tok::identifier, tok::kw_const)) ||
          (Left.is(tok::l_paren) && !Right.is(tok::r_paren)) ||
          (Left.is(TT_TemplateOpener) && !Right.is(TT_TemplateCloser));
 }
 
 void TokenAnnotator::printDebugInfo(const AnnotatedLine &Line) {
-  llvm::errs() << "AnnotatedTokens:\n";
+  llvm::errs() << "AnnotatedTokens(L=" << Line.Level << "):\n";
   const FormatToken *Tok = Line.First;
   while (Tok) {
     llvm::errs() << " M=" << Tok->MustBreakBefore
                  << " C=" << Tok->CanBreakBefore
                  << " T=" << getTokenTypeName(Tok->Type)
                  << " S=" << Tok->SpacesRequiredBefore
                  << " B=" << Tok->BlockParameterCount
                  << " BK=" << Tok->BlockKind

cfe/trunk/lib/Format/UnwrappedLineParser.h

private:
   void parseObjCInterfaceOrImplementation();
   void parseObjCProtocol();
   void parseJavaScriptEs6ImportExport();
   bool tryToParseLambda();
   bool tryToParseLambdaIntroducer();
   void tryToParseJSFunction();
   void addUnwrappedLine();
   bool eof() const;
-  void nextToken();
+  // LevelDifference is the difference of levels after and before the current
+  // token. For example:
+  // - if the token is '{' and opens a block, LevelDifference is 1.
+  // - if the token is '}' and closes a block, LevelDifference is -1.
+  void nextToken(int LevelDifference = 0);
+  void readToken(int LevelDifference = 0);
   const FormatToken *getPreviousToken();
-  void readToken();
 
   // 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.
   //

cfe/trunk/lib/Format/UnwrappedLineParser.cpp

 void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
                                      bool MunchSemi) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
          "'{' or macro block token expected");
   const bool MacroBlock = FormatTok->is(TT_MacroBlockBegin);
   FormatTok->BlockKind = BK_Block;
 
   unsigned InitialLevel = Line->Level;
-  nextToken();
+  nextToken(/*LevelDifference=*/AddLevel ? 1 : 0);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
 
   addUnwrappedLine();
   size_t OpeningLineIndex = CurrentLines->empty()
                                 ? (UnwrappedLine::kInvalidIndex)
                                 : (CurrentLines->size() - 1);
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.
+  // Munch the closing brace.
+  nextToken(/*LevelDifference=*/AddLevel ? -1 : 0);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
 
   if (MunchSemi && FormatTok->Tok.is(tok::semi))
     nextToken();
   Line->MatchingOpeningBlockLineIndex = OpeningLineIndex;
+  Line->Level = InitialLevel;
   if (OpeningLineIndex != UnwrappedLine::kInvalidIndex) {
     // Update the opening line to add the forward reference as well
     (*CurrentLines)[OpeningLineIndex].MatchingOpeningBlockLineIndex =
             CurrentLines->size() - 1;
   }
 }
 
 static bool isGoogScope(const UnwrappedLine &Line) {
if (isOnNewLine(**I) && JustComments && !(*I)->ContinuesLineCommentSection)
       addUnwrappedLine();
     pushToken(*I);
   }
   if (NewlineBeforeNext && JustComments)
     addUnwrappedLine();
   CommentsBeforeNextToken.clear();
 }
 
-void UnwrappedLineParser::nextToken() {
+void UnwrappedLineParser::nextToken(int LevelDifference) {
   if (eof())
     return;
   flushComments(isOnNewLine(*FormatTok));
   pushToken(FormatTok);
   if (Style.Language != FormatStyle::LK_JavaScript)
-    readToken();
+    readToken(LevelDifference);
   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(int LevelDifference) {
   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);
+      assert((LevelDifference >= 0 ||
+              static_cast<unsigned>(-LevelDifference) <= Line->Level) &&
+             "LevelDifference makes Line->Level negative");
+      Line->Level += LevelDifference;
       // 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 ||

cfe/trunk/unittests/Format/FormatTestComments.cpp

EXPECT_EQ("int f(int i) {\n"
                    "    ++i;\n"
                    "  }\n"
                    "  // comment\n"
                    "#ifdef A\n"
                    "int j;\n"
                    "#endif\n"
                    "}"));
 
+  EXPECT_EQ("int f(int i) {\n"
+            "  if (true) {\n"
+            "    i++;\n"
+            "  } else {\n"
+            "    // comment in else\n"
+            "#ifdef A\n"
+            "    j++;\n"
+            "#endif\n"
+            "  }\n"
+            "}",
+            format("int f(int i) {\n"
+                   "  if (true) {\n"
+                   "    i++;\n"
+                   "  } else {\n"
+                   "  // comment in else\n"
+                   "#ifdef A\n"
+                   "    j++;\n"
+                   "#endif\n"
+                   "  }\n"
+                   "}"));
+
+  EXPECT_EQ("int f(int i) {\n"
+            "  if (true) {\n"
+            "    i++;\n"
+            "  } else {\n"
+            "    /* comment in else */\n"
+            "#ifdef A\n"
+            "    j++;\n"
+            "#endif\n"
+            "  }\n"
+            "}",
+            format("int f(int i) {\n"
+                   "  if (true) {\n"
+                   "    i++;\n"
+                   "  } else {\n"
+                   "  /* comment in else */\n"
+                   "#ifdef A\n"
+                   "    j++;\n"
+                   "#endif\n"
+                   "  }\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"
EXPECT_EQ("int f(int i) {\n"
                    "  }\n"
                    "  // comment\n"
                    "\n"
                    "#ifdef A\n"
                    "  int j;\n"
                    "#endif\n"
                    "}"));
 
+  EXPECT_EQ("int f(int i) {\n"
+            "  if (true) {\n"
+            "    i++;\n"
+            "  } else {\n"
+            "    // comment in else\n"
+            "\n"
+            "#ifdef A\n"
+            "    j++;\n"
+            "#endif\n"
+            "  }\n"
+            "}",
+            format("int f(int i) {\n"
+                   "  if (true) {\n"
+                   "    i++;\n"
+                   "  } else {\n"
+                   "// comment in else\n"
+                   "\n"
+                   "#ifdef A\n"
+                   "    j++;\n"
+                   "#endif\n"
+                   "  }\n"
+                   "}"));
+
+  EXPECT_EQ("int f(int i) {\n"
+            "  if (true) {\n"
+            "    i++;\n"
+            "  } else {\n"
+            "    /* comment in else */\n"
+            "\n"
+            "#ifdef A\n"
+            "    j++;\n"
+            "#endif\n"
+            "  }\n"
+            "}",
+            format("int f(int i) {\n"
+                   "  if (true) {\n"
+                   "    i++;\n"
+                   "  } else {\n"
+                   "/* comment in else */\n"
+                   "\n"
+                   "#ifdef A\n"
+                   "    j++;\n"
+                   "#endif\n"
+                   "  }\n"
+                   "}"));
+
   // Align with the preprocessor directive if the comment was originally aligned
-  // with the preprocessor directive.
+  // with the preprocessor directive and there is no newline between the comment
+  // and 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"
EXPECT_EQ("int f(int i) {\n"
                    "   if (true) {\n"
                    "    ++i;\n"
                    "  }\n"
                    "// comment\n"
                    "#ifdef A\n"
                    "  int j;\n"
                    "#endif\n"
                    "}"));
+
+  EXPECT_EQ("int f(int i) {\n"
+            "  if (true) {\n"
+            "    i++;\n"
+            "  } else {\n"
+            "// comment in else\n"
+            "#ifdef A\n"
+            "    j++;\n"
+            "#endif\n"
+            "  }\n"
+            "}",
+            format("int f(int i) {\n"
+                   "  if (true) {\n"
+                   "    i++;\n"
+                   "  } else {\n"
+                   " // comment in else\n"
+                   " #ifdef A\n"
+                   "    j++;\n"
+                   "#endif\n"
+                   "  }\n"
+                   "}"));
+
+  EXPECT_EQ("int f(int i) {\n"
+            "  if (true) {\n"
+            "    i++;\n"
+            "  } else {\n"
+            "/* comment in else */\n"
+            "#ifdef A\n"
+            "    j++;\n"
+            "#endif\n"
+            "  }\n"
+            "}",
+            format("int f(int i) {\n"
+                   "  if (true) {\n"
+                   "    i++;\n"
+                   "  } else {\n"
+                   " /* comment in else */\n"
+                   " #ifdef A\n"
+                   "    j++;\n"
+                   "#endif\n"
+                   "  }\n"
+                   "}"));
 }
 
 TEST_F(FormatTestComments, SplitsLongLinesInComments) {
   EXPECT_EQ("/* This is a long\n"
             " * comment that\n"
             " * doesn't\n"
             " * fit on one line.\n"
             " */",