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

[Krasimir Georgiev via cfe-commits]

Author: krasimir
Date: Mon May 22 05:07:56 2017
New Revision: 303541

URL: http://llvm.org/viewvc/llvm-project?rev=303541&view=rev
Log:
[clang-format] Keep trailing preprocessor line comments separate from the following section comments

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.

Reviewers: djasper

Reviewed By: djasper

Subscribers: klimek, cfe-commits

Differential Revision: https://reviews.llvm.org/D33394

Modified:
    cfe/trunk/lib/Format/UnwrappedLineParser.cpp
    cfe/trunk/unittests/Format/FormatTestComments.cpp

Modified: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=303541&r1=303540&r2=303541&view=diff
==============================================================================
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp (original)
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp Mon May 22 05:07:56 2017
@@ -60,6 +60,21 @@ static bool isLineComment(const FormatTo
          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,
@@ -101,8 +116,8 @@ public:
 private:
   bool eof() {
     return Token && Token->HasUnescapedNewline &&
-           !(PreviousToken && isLineComment(*PreviousToken) &&
-             isLineComment(*Token) && Token->NewlinesBefore == 1);
+           !continuesLineComment(*Token, PreviousToken,
+                                 /*MinColumnToken=*/PreviousToken);
   }
 
   FormatToken *getFakeEOF() {
@@ -2106,9 +2121,9 @@ bool UnwrappedLineParser::isOnNewLine(co
 
 // Checks if \p FormatTok is a line comment that continues the line comment
 // section on \p Line.
-static bool continuesLineComment(const FormatToken &FormatTok,
-                                 const UnwrappedLine &Line,
-                                 llvm::Regex &CommentPragmasRegex) {
+static bool continuesLineCommentSection(const FormatToken &FormatTok,
+                                        const UnwrappedLine &Line,
+                                        llvm::Regex &CommentPragmasRegex) {
   if (Line.Tokens.empty())
     return false;
 
@@ -2207,12 +2222,8 @@ static bool continuesLineComment(const F
     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) {
@@ -2230,7 +2241,7 @@ void UnwrappedLineParser::flushComments(
     // 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);
@@ -2263,7 +2274,7 @@ void UnwrappedLineParser::distributeComm
     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
@@ -2301,7 +2312,7 @@ void UnwrappedLineParser::distributeComm
       FormatTok->ContinuesLineCommentSection = false;
     } else {
       FormatTok->ContinuesLineCommentSection =
-          continuesLineComment(*FormatTok, *Line, CommentPragmasRegex);
+          continuesLineCommentSection(*FormatTok, *Line, CommentPragmasRegex);
     }
     if (!FormatTok->ContinuesLineCommentSection &&
         (isOnNewLine(*FormatTok) || FormatTok->IsFirst)) {

Modified: cfe/trunk/unittests/Format/FormatTestComments.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTestComments.cpp?rev=303541&r1=303540&r2=303541&view=diff
==============================================================================
--- cfe/trunk/unittests/Format/FormatTestComments.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTestComments.cpp Mon May 22 05:07:56 2017
@@ -1020,6 +1020,38 @@ TEST_F(FormatTestComments, SplitsLongLin
                    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"