[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

2264

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

2265

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

lib/Format/UnwrappedLineParser.h

118

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

[djasper]

lib/Format/UnwrappedLineParser.cpp

2264

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

2265

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.

2267

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

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

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

Done. Thanks!

[klimek]

lib/Format/UnwrappedLineParser.h

121–123

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

Maybe distributeCommentsAccordingToAlignment?

[klimek]

lib/Format/UnwrappedLineParser.h

121–123

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

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

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

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.

lib/Format/UnwrappedLineParser.h

private:
   bool tryToParseLambda();
   bool tryToParseLambdaIntroducer();
   void tryToParseJSFunction();
   void addUnwrappedLine();
   bool eof() const;
   void nextToken();
   const FormatToken *getPreviousToken();
   void readToken();
+  void analyzeCommentAlignment(const SmallVectorImpl<FormatToken *> &Comments,
+                               const FormatToken *NextTok);
   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.
   void conditionalCompilationCondition(bool Unreachable);
   void conditionalCompilationStart(bool Unreachable);
   void conditionalCompilationAlternative();
   void conditionalCompilationEnd();
 

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::analyzeCommentAlignment(
+    const SmallVectorImpl<FormatToken *> &Comments,
+    const FormatToken *NextTok) {
+  // Decides which comment tokens should be added to the current line and which
+  // should be added as comments before the next token.
+  //
+  // 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)) {
+      analyzeCommentAlignment(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)) {
+      analyzeCommentAlignment(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());
+
+  analyzeCommentAlignment(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

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