[clang-format] Break before a sequence of line comments aligned with the next line.

[krasimir]

Summary

Make the comment alignment respect sections of line comments originally alinged
with the next token. Until now the decision how to break a continuous sequence
of line comments into sections was taken without reference to the next token.

source:

class A {
public: // comment about public
  // comment about a
  int a;
}

format before:

class A {
public: // comment about public
        // comment about a
  int a;
}

format after:

class A {
public: // comment about public
  // comment about a
  int a;
}

[krasimir]

I feel that this is a bit too rough now; any suggestions on improving the architecture/design of this patch are welcome!

lib/Format/UnwrappedLineParser.cpp

2206 ↗

(On Diff #87387)

Any suggestions on how to improve the code quality a bit if we stick with this design?

2207 ↗

(On Diff #87387)

Need to add comments about this if we decide we may go with this.

lib/Format/UnwrappedLineParser.h

118 ↗

(On Diff #87387)

@djasper: considering what this code does, what would be a better name of this?

[djasper]

lib/Format/UnwrappedLineParser.cpp

2206 ↗

(On Diff #87387)

I think comments might actually help here, but also this isn't too bad. As in, when writing the comments, it might become more obvious how to write this better.

2207 ↗

(On Diff #87387)

Yeah.. Commented comment handling is best comment handling :-D

2210 ↗

(On Diff #87387)

I think we use "unsigned" in most of these loops.

And to avoid and edge conditions put this at the top of the function:

if (Comments.empty())
  return;

lib/Format/UnwrappedLineParser.h

118 ↗

(On Diff #87387)

Maybe analyzeCommentAlignment? Or determineCommentAlignment?

[klimek]

I think this looks pretty good. More comments would help :) Also, organize is spelled with a 'z' in American.

[krasimir]

• Address review comments.

[klimek]

lib/Format/UnwrappedLineParser.cpp

2207–2208 ↗

(On Diff #87410)

Put these 2 lines and what it does to the arguments into the header.

[krasimir]

• Moved method summary from the implementation to the declaration.

[krasimir]

lib/Format/UnwrappedLineParser.cpp

2207–2208 ↗

(On Diff #87410)

Done. Thanks!

[klimek]

lib/Format/UnwrappedLineParser.h

121–123 ↗

(On Diff #87424)

Given this, I'd perhaps call this addSection or addCommentSection or something similar? analyze sounds like it doesn't change the state of the class...

[krasimir]

lib/Format/UnwrappedLineParser.h

121–123 ↗

(On Diff #87424)

Maybe distributeCommentsAccordingToAlignment?

[klimek]

lib/Format/UnwrappedLineParser.h

121–123 ↗

(On Diff #87424)

I don't think "AccordingToAlignment" needs to be in the name - it's fine that that's in the comment, and it's kind of a detail of what this does.

The important thing at the call site is that this adds comments to the current unwrapped line, right?

[krasimir]

lib/Format/UnwrappedLineParser.h

121–123 ↗

(On Diff #87424)

That's just half of it: it may add comments to the current line, and it may add comments to the sequence of tokens preceding the next token with conceptually belong to the next token and will be put in a new line if the next token has to be put on a new line.
After a while, there is "flushComments" which takes care of the mentioned comments before next token.

[klimek]

lib/Format/UnwrappedLineParser.h

121–123 ↗

(On Diff #87424)

Sure, but the important part seems to be that it decides which comment tokens belong into the current line?

[krasimir]

• Invent better name for comment analysis.

[krasimir]

• Updated call sites.

lib/Format/UnwrappedLineParser.h

121–123 ↗

(On Diff #87424)

Renamed. While at it, I added a line of comments for "flushComments" since now we have two methods with similar enough names to cause a little confusion with no comments.

[klimek]

lg.

I'd probably still call it consumeComments or something, as we require a flush afterwards for the unconsumed comments, but I don't feel too strongly about that.

cfe/trunk/lib/Format/UnwrappedLineParser.h

private:
   bool tryToParseLambda();
   bool tryToParseLambdaIntroducer();
   void tryToParseJSFunction();
   void addUnwrappedLine();
   bool eof() const;
   void nextToken();
   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.
+  //
+  // NextTok specifies the next token. A null pointer NextTok is supported, and
+  // signifies either the absense of a next token, or that the next token
+  // shouldn't be taken into accunt for the analysis.
+  void distributeComments(const SmallVectorImpl<FormatToken *> &Comments,
+                          const FormatToken *NextTok);
+
+  // Adds the comment preceding the next token to unwrapped lines.
   void flushComments(bool NewlineBeforeNext);
   void pushToken(FormatToken *Tok);
   void calculateBraceTypes(bool ExpectClassBody = false);
 
   // Marks a conditional compilation edge (for example, an '#if', '#ifdef',
   // '#else' or merge conflict marker). If 'Unreachable' is true, assumes
   // this branch either cannot be taken (for example '#if false'), or should
   // not be taken in this round.

cfe/trunk/lib/Format/UnwrappedLineParser.cpp

 const FormatToken *UnwrappedLineParser::getPreviousToken() {
   // FIXME: This is a dirty way to access the previous token. Find a better
   // solution.
   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:
+  //
+  // 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:
+  //
+  // int a; // line about a
+  //   // line 1 about b
+  //   // line 2 about b
+  //   int b;
+  //
+  // the two lines about b form a maximal trail, so there are two sections, the
+  // first one consisting of the single comment "// line about a" and the
+  // second one consisting of the next two comments.
+  if (Comments.empty())
+    return;
+  bool ShouldPushCommentsInCurrentLine = true;
+  bool HasTrailAlignedWithNextToken = false;
+  unsigned StartOfTrailAlignedWithNextToken = 0;
+  if (NextTok) {
+    // We are skipping the first element intentionally.
+    for (unsigned i = Comments.size() - 1; i > 0; --i) {
+      if (Comments[i]->OriginalColumn == NextTok->OriginalColumn) {
+        HasTrailAlignedWithNextToken = true;
+        StartOfTrailAlignedWithNextToken = i;
+      }
+    }
+  }
+  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);
+    }
+    if (!FormatTok->ContinuesLineCommentSection &&
+        (isOnNewLine(*FormatTok) || FormatTok->IsFirst)) {
+      ShouldPushCommentsInCurrentLine = false;
+    }
+    if (ShouldPushCommentsInCurrentLine) {
+      pushToken(FormatTok);
+    } else {
+      CommentsBeforeNextToken.push_back(FormatTok);
+    }
+  }
+}
+
 void UnwrappedLineParser::readToken() {
-  bool CommentsInCurrentLine = true;
+  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);
       // 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));
while (FormatTok->Type == TT_ConflictStart ||
       FormatTok->MustBreakBefore = true;
     }
 
     if (!PPStack.empty() && (PPStack.back() == PP_Unreachable) &&
         !Line->InPPDirective) {
       continue;
     }
 
-    if (!FormatTok->Tok.is(tok::comment))
+    if (!FormatTok->Tok.is(tok::comment)) {
+      distributeComments(Comments, FormatTok);
+      Comments.clear();
       return;
-    FormatTok->ContinuesLineCommentSection =
-        continuesLineComment(*FormatTok, *Line, CommentPragmasRegex);
-    if (!FormatTok->ContinuesLineCommentSection &&
-        (isOnNewLine(*FormatTok) || FormatTok->IsFirst)) {
-      CommentsInCurrentLine = false;
-    }
-    if (CommentsInCurrentLine) {
-      pushToken(FormatTok);
-    } else {
-      CommentsBeforeNextToken.push_back(FormatTok);
     }
+
+    Comments.push_back(FormatTok);
   } while (!eof());
+
+  distributeComments(Comments, nullptr);
+  Comments.clear();
 }
 
 void UnwrappedLineParser::pushToken(FormatToken *Tok) {
   Line->Tokens.push_back(UnwrappedLineNode(Tok));
   if (MustBreakBeforeNextToken) {
     Line->Tokens.back().Tok->MustBreakBefore = true;
     MustBreakBeforeNextToken = false;
   }
 }
 
 } // end namespace format
 } // end namespace clang

cfe/trunk/unittests/Format/FormatTest.cpp

EXPECT_EQ("int ab; // line\n"
             "        // long\n"
             "\n"
             "// long",
             format("int ab; // line\n"
                    "int a; // long long\n"
                    "\n"
                    "// long",
                    getLLVMStyleWithColumns(15)));
+
+  // Align comment line sections aligned with the next token with the next
+  // token.
+  EXPECT_EQ("class A {\n"
+            "public: // public comment\n"
+            "  // comment about a\n"
+            "  int a;\n"
+            "};",
+            format("class A {\n"
+                   "public: // public comment\n"
+                   "  // comment about a\n"
+                   "  int a;\n"
+                   "};",
+                   getLLVMStyleWithColumns(40)));
+  EXPECT_EQ("class A {\n"
+            "public: // public comment 1\n"
+            "        // public comment 2\n"
+            "  // comment 1 about a\n"
+            "  // comment 2 about a\n"
+            "  int a;\n"
+            "};",
+            format("class A {\n"
+                   "public: // public comment 1\n"
+                   "   // public comment 2\n"
+                   "  // comment 1 about a\n"
+                   "  // comment 2 about a\n"
+                   "  int a;\n"
+                   "};",
+                   getLLVMStyleWithColumns(40)));
+  EXPECT_EQ("int f(int n) { // comment line 1 on f\n"
+            "               // comment line 2 on f\n"
+            "  // comment line 1 before return\n"
+            "  // comment line 2 before return\n"
+            "  return n; // comment line 1 on return\n"
+            "            // comment line 2 on return\n"
+            "  // comment line 1 after return\n"
+            "}",
+            format("int f(int n) { // comment line 1 on f\n"
+                   "   // comment line 2 on f\n"
+                   "  // comment line 1 before return\n"
+                   "  // comment line 2 before return\n"
+                   "  return n; // comment line 1 on return\n"
+                   "   // comment line 2 on return\n"
+                   "  // comment line 1 after return\n"
+                   "}",
+                   getLLVMStyleWithColumns(40)));
+  EXPECT_EQ("int f(int n) {\n"
+            "  switch (n) { // comment line 1 on switch\n"
+            "               // comment line 2 on switch\n"
+            "  // comment line 1 before case 1\n"
+            "  // comment line 2 before case 1\n"
+            "  case 1: // comment line 1 on case 1\n"
+            "          // comment line 2 on case 1\n"
+            "    // comment line 1 before return 1\n"
+            "    // comment line 2 before return 1\n"
+            "    return 1; // comment line 1 on return 1\n"
+            "              // comment line 2 on return 1\n"
+            "  // comment line 1 before default\n"
+            "  // comment line 2 before default\n"
+            "  default: // comment line 1 on default\n"
+            "           // comment line 2 on default\n"
+            "    // comment line 1 before return 2\n"
+            "    return 2 * f(n - 1); // comment line 1 on return 2\n"
+            "                         // comment line 2 on return 2\n"
+            "    // comment line 1 after return\n"
+            "    // comment line 2 after return\n"
+            "  }\n"
+            "}",
+            format("int f(int n) {\n"
+                   "  switch (n) { // comment line 1 on switch\n"
+                   "              // comment line 2 on switch\n"
+                   "    // comment line 1 before case 1\n"
+                   "    // comment line 2 before case 1\n"
+                   "    case 1: // comment line 1 on case 1\n"
+                   "              // comment line 2 on case 1\n"
+                   "    // comment line 1 before return 1\n"
+                   "    // comment line 2 before return 1\n"
+                   "    return 1;  // comment line 1 on return 1\n"
+                   "             // comment line 2 on return 1\n"
+                   "    // comment line 1 before default\n"
+                   "    // comment line 2 before default\n"
+                   "    default:   // comment line 1 on default\n"
+                   "                // comment line 2 on default\n"
+                   "    // comment line 1 before return 2\n"
+                   "    return 2 * f(n - 1); // comment line 1 on return 2\n"
+                   "                        // comment line 2 on return 2\n"
+                   "    // comment line 1 after return\n"
+                   "     // comment line 2 after return\n"
+                   "  }\n"
+                   "}",
+                   getLLVMStyleWithColumns(80)));
+
+  // If all the lines in a sequence of line comments are aligned with the next
+  // token, the first line belongs to the previous token and the other lines
+  // belong to the next token.
+  EXPECT_EQ("int a; // line about a\n"
+            "long b;",
+            format("int a; // line about a\n"
+                   "       long b;",
+                   getLLVMStyleWithColumns(80)));
+  EXPECT_EQ("int a; // line about a\n"
+            "// line about b\n"
+            "long b;",
+            format("int a; // line about a\n"
+                   "       // line about b\n"
+                   "       long b;",
+                   getLLVMStyleWithColumns(80)));
+  EXPECT_EQ("int a; // line about a\n"
+            "// line 1 about b\n"
+            "// line 2 about b\n"
+            "long b;",
+            format("int a; // line about a\n"
+                   "       // line 1 about b\n"
+                   "       // line 2 about b\n"
+                   "       long b;",
+                   getLLVMStyleWithColumns(80)));
 }
 } // end namespace
 } // end namespace format
 } // end namespace clang