[clang-format] Keep trailing preprocessor line comments separate from the following section comments

[krasimir]

Summary

r303415 changed the way a sequence of line comments following a preprocessor
macro is handled, which has the unfortunate effect of aligning a trailing
preprocessor line comment and following unrelated section comments, so:

#ifdef A // comment about A
// section comment
#endif

gets turned into:

#ifdef A // comment about A
         // section comment
#endif

This patch fixes this by additionally checking the original start columns of
the line comments.

[djasper]

lib/Format/UnwrappedLineParser.cpp

106 ↗

(On Diff #99722)

Any chance of moving this logic to distributeComments? I think that's the place we previously tried to encapsulate this logic in. If not, this is fine for now.

unittests/Format/FormatTestComments.cpp

1030 ↗

(On Diff #99722)

Would it make sense to also add a test case in which the #ifdef comment is actually continued?

[krasimir]

• Address review comments

[krasimir]

lib/Format/UnwrappedLineParser.cpp

106 ↗

(On Diff #99722)

Because of the way parsing around macros works, moving this to distributeComments is gonna be super-tricky: I tried this approach first for a while and gave up. However, I can extract the common logic here and in continuesLineComment at least.

[djasper]

lib/Format/UnwrappedLineParser.cpp

119 ↗

(On Diff #99729)

Remove parentheses?

106 ↗

(On Diff #99722)

Yeah. Much nicer. Thank you.

[krasimir]

• Remove parentheses

cfe/trunk/lib/Format/UnwrappedLineParser.cpp

private:
   std::vector<bool> &Stack;
 };
 
 static bool isLineComment(const FormatToken &FormatTok) {
   return FormatTok.is(tok::comment) &&
          FormatTok.TokenText.startswith("//");
 }
 
+// Checks if \p FormatTok is a line comment that continues the line comment
+// \p Previous. The original column of \p MinColumnToken is used to determine
+// whether \p FormatTok is indented enough to the right to continue \p Previous.
+static bool continuesLineComment(const FormatToken &FormatTok,
+                                 const FormatToken *Previous,
+                                 const FormatToken *MinColumnToken) {
+  if (!Previous || !MinColumnToken)
+    return false;
+  unsigned MinContinueColumn =
+      MinColumnToken->OriginalColumn + (isLineComment(*MinColumnToken) ? 0 : 1);
+  return isLineComment(FormatTok) && FormatTok.NewlinesBefore == 1 &&
+         isLineComment(*Previous) &&
+         FormatTok.OriginalColumn >= MinContinueColumn;
+}
+
 class ScopedMacroState : public FormatTokenSource {
 public:
   ScopedMacroState(UnwrappedLine &Line, FormatTokenSource *&TokenSource,
                    FormatToken *&ResetToken)
       : Line(Line), TokenSource(TokenSource), ResetToken(ResetToken),
         PreviousLineLevel(Line.Level), PreviousTokenSource(TokenSource),
         Token(nullptr), PreviousToken(nullptr) {
     TokenSource = this;
FormatToken *setPosition(unsigned Position) override {
     PreviousToken = nullptr;
     Token = PreviousTokenSource->setPosition(Position);
     return Token;
   }
 
 private:
   bool eof() {
     return Token && Token->HasUnescapedNewline &&
-           !(PreviousToken && isLineComment(*PreviousToken) &&
-             isLineComment(*Token) && Token->NewlinesBefore == 1);
+           !continuesLineComment(*Token, PreviousToken,
+                                 /*MinColumnToken=*/PreviousToken);
   }
 
   FormatToken *getFakeEOF() {
     static bool EOFInitialized = false;
     static FormatToken FormatTok;
     if (!EOFInitialized) {
       FormatTok.Tok.startToken();
       FormatTok.Tok.setKind(tok::eof);

 
 bool UnwrappedLineParser::isOnNewLine(const FormatToken &FormatTok) {
   return (Line->InPPDirective || FormatTok.HasUnescapedNewline) &&
          FormatTok.NewlinesBefore > 0;
 }
 
 // Checks if \p FormatTok is a line comment that continues the line comment
 // section on \p Line.
-static bool continuesLineComment(const FormatToken &FormatTok,
+static bool continuesLineCommentSection(const FormatToken &FormatTok,
                                         const UnwrappedLine &Line,
                                         llvm::Regex &CommentPragmasRegex) {
   if (Line.Tokens.empty())
     return false;
 
   StringRef IndentContent = FormatTok.TokenText;
   if (FormatTok.TokenText.startswith("//") ||
       FormatTok.TokenText.startswith("/*"))
     IndentContent = FormatTok.TokenText.substr(2);
   if (CommentPragmasRegex.match(IndentContent))
for (const UnwrappedLineNode &Node : Line.Tokens) {
     if (Node.Tok->NewlinesBefore > 0) {
       MinColumnToken = Node.Tok;
     }
   }
   if (PreviousToken && PreviousToken->is(tok::l_brace)) {
     MinColumnToken = PreviousToken;
   }
 
-  unsigned MinContinueColumn =
-      MinColumnToken->OriginalColumn +
-      (isLineComment(*MinColumnToken) ? 0 : 1);
-  return isLineComment(FormatTok) && FormatTok.NewlinesBefore == 1 &&
-         isLineComment(*(Line.Tokens.back().Tok)) &&
-         FormatTok.OriginalColumn >= MinContinueColumn;
+  return continuesLineComment(FormatTok, /*Previous=*/Line.Tokens.back().Tok,
+                              MinColumnToken);
 }
 
 void UnwrappedLineParser::flushComments(bool NewlineBeforeNext) {
   bool JustComments = Line->Tokens.empty();
   for (SmallVectorImpl<FormatToken *>::const_iterator
            I = CommentsBeforeNextToken.begin(),
            E = CommentsBeforeNextToken.end();
        I != E; ++I) {
     // Line comments that belong to the same line comment section are put on the
     // same line since later we might want to reflow content between them.
     // Additional fine-grained breaking of line comment sections is controlled
     // by the class BreakableLineCommentSection in case it is desirable to keep
     // several line comment sections in the same unwrapped line.
     //
     // FIXME: Consider putting separate line comment sections as children to the
     // unwrapped line instead.
     (*I)->ContinuesLineCommentSection =
-        continuesLineComment(**I, *Line, CommentPragmasRegex);
+        continuesLineCommentSection(**I, *Line, CommentPragmasRegex);
     if (isOnNewLine(**I) && JustComments && !(*I)->ContinuesLineCommentSection)
       addUnwrappedLine();
     pushToken(*I);
   }
   if (NewlineBeforeNext && JustComments)
     addUnwrappedLine();
   CommentsBeforeNextToken.clear();
 }
if (!Line || Line->Tokens.empty())
     return nullptr;
   return Line->Tokens.back().Tok;
 }
 
 void UnwrappedLineParser::distributeComments(
     const SmallVectorImpl<FormatToken *> &Comments,
     const FormatToken *NextTok) {
   // Whether or not a line comment token continues a line is controlled by
-  // the method continuesLineComment, with the following caveat:
+  // the method continuesLineCommentSection, with the following caveat:
   //
   // Define a trail of Comments to be a nonempty proper postfix of Comments such
   // that each comment line from the trail is aligned with the next token, if
   // the next token exists. If a trail exists, the beginning of the maximal
   // trail is marked as a start of a new comment section.
   //
   // For example in this code:
   //
void UnwrappedLineParser::distributeComments(
   }
   for (unsigned i = 0, e = Comments.size(); i < e; ++i) {
     FormatToken *FormatTok = Comments[i];
     if (HasTrailAlignedWithNextToken &&
         i == StartOfTrailAlignedWithNextToken) {
       FormatTok->ContinuesLineCommentSection = false;
     } else {
       FormatTok->ContinuesLineCommentSection =
-          continuesLineComment(*FormatTok, *Line, CommentPragmasRegex);
+          continuesLineCommentSection(*FormatTok, *Line, CommentPragmasRegex);
     }
     if (!FormatTok->ContinuesLineCommentSection &&
         (isOnNewLine(*FormatTok) || FormatTok->IsFirst)) {
       ShouldPushCommentsInCurrentLine = false;
     }
     if (ShouldPushCommentsInCurrentLine) {
       pushToken(FormatTok);
     } else {

cfe/trunk/unittests/Format/FormatTestComments.cpp

EXPECT_EQ("#define X          \\\n"
             "   * line */       \\\n"
             "  A + B",
             format("#define X \\\n"
                    "  /* Macro comment with a long  line */ \\\n"
                    "  A + B",
                    getLLVMStyleWithColumns(20)));
 }
 
+TEST_F(FormatTestComments, KeepsTrailingPPCommentsAndSectionCommentsSeparate) {
+  verifyFormat("#ifdef A // line about A\n"
+               "// section comment\n"
+               "#endif",
+               getLLVMStyleWithColumns(80));
+  verifyFormat("#ifdef A // line 1 about A\n"
+               "         // line 2 about A\n"
+               "// section comment\n"
+               "#endif",
+               getLLVMStyleWithColumns(80));
+  EXPECT_EQ("#ifdef A // line 1 about A\n"
+            "         // line 2 about A\n"
+            "// section comment\n"
+            "#endif",
+            format("#ifdef A // line 1 about A\n"
+                   "          // line 2 about A\n"
+                   "// section comment\n"
+                   "#endif",
+                   getLLVMStyleWithColumns(80)));
+  verifyFormat("int f() {\n"
+               "  int i;\n"
+               "#ifdef A // comment about A\n"
+               "  // section comment 1\n"
+               "  // section comment 2\n"
+               "  i = 2;\n"
+               "#else // comment about #else\n"
+               "  // section comment 3\n"
+               "  i = 4;\n"
+               "#endif\n"
+               "}", getLLVMStyleWithColumns(80));
+}
+
 TEST_F(FormatTestComments, CommentsInStaticInitializers) {
   EXPECT_EQ(
       "static SomeType type = {aaaaaaaaaaaaaaaaaaaa, /* comment */\n"
       "                        aaaaaaaaaaaaaaaaaaaa /* comment */,\n"
       "                        /* comment */ aaaaaaaaaaaaaaaaaaaa,\n"
       "                        aaaaaaaaaaaaaaaaaaaa, // comment\n"
       "                        aaaaaaaaaaaaaaaaaaaa};",
       format("static SomeType type = { aaaaaaaaaaaaaaaaaaaa  ,  /* comment */\n"