[clang-format] Fix comment aligning when there are changes within the comment

[bkramer]

Summary

As soon as a comment had whitespace changes inside of the token, we
couldn't identify the whole comment as a trailing comment anymore and
alignment stopped working. Add a new boolean to Change for this special
case and fix trailing comment identification to use it.

Before this fix

int xy; a
int z; b

became

int xy; a
int z; b

with this patch we immediately get to:

int xy; a
int z; b

[djasper]

lib/Format/WhitespaceManager.h

145 ↗

(On Diff #44466)

I find the term "continuation" a bit confusing here. How about something like "IsInsideToken"?

[klimek]

lib/Format/WhitespaceManager.h

145 ↗

(On Diff #44466)

"Token" sounds like it could be true for things outside comments. Perhaps "InComment"?

[bkramer]

Why not both?

[djasper]

lib/Format/WhitespaceManager.h

145 ↗

(On Diff #44466)

So why not store the more generic information here? We already have the extra information whether this is a line comment or not stored in the token kind.

[bkramer]

• Renamed flag to IsInsideToken and enabled it for all in-token replacements.
• TokenLength now gets updated to contain all changes on the same line if they're in the same token.

[djasper]

I think this looks good, but I'd also like Manuel to take a look.

[bkramer]

• Moved newline check into replaceWhitespaceInToken
• Removed duplicated TokenLength computation

[klimek]

lg

lib/Format/WhitespaceManager.h

113 ↗

(On Diff #44513)

Forgot one replacement of the original name.

143 ↗

(On Diff #44513)

... or starting a new line.

cfe/trunk/lib/Format/WhitespaceManager.h

struct Change {
     /// and \p CurrentLinePrefix.
     ///
     /// \p StartOfTokenColumn and \p InPPDirective will be used to lay out
     /// trailing comments and escaped newlines.
     Change(bool CreateReplacement, SourceRange OriginalWhitespaceRange,
            unsigned IndentLevel, int Spaces, unsigned StartOfTokenColumn,
            unsigned NewlinesBefore, StringRef PreviousLinePostfix,
            StringRef CurrentLinePrefix, tok::TokenKind Kind,
-           bool ContinuesPPDirective, bool IsStartOfDeclName);
+           bool ContinuesPPDirective, bool IsStartOfDeclName,
+           bool IsInsideToken);
 
     bool CreateReplacement;
     // Changes might be in the middle of a token, so we cannot just keep the
     // FormatToken around to query its information.
     SourceRange OriginalWhitespaceRange;
     unsigned StartOfTokenColumn;
     unsigned NewlinesBefore;
     std::string PreviousLinePostfix;
struct Change {
 
     // The number of spaces in front of the token or broken part of the token.
     // This will be adapted when aligning tokens.
     // Can be negative to retain information about the initial relative offset
     // of the lines in a block comment. This is used when aligning trailing
     // comments. Uncompensated negative offset is truncated to 0.
     int Spaces;
 
+    // If this change is inside of a token but not at the start of the token or
+    // directly after a newline.
+    bool IsInsideToken;
+
     // \c IsTrailingComment, \c TokenLength, \c PreviousEndOfTokenColumn and
     // \c EscapedNewlineColumn will be calculated in
     // \c calculateLineBreakInformation.
     bool IsTrailingComment;
     unsigned TokenLength;
     unsigned PreviousEndOfTokenColumn;
     unsigned EscapedNewlineColumn;
 

cfe/trunk/lib/Format/WhitespaceManager.cpp

return SourceMgr.isBeforeInTranslationUnit(
       C2.OriginalWhitespaceRange.getBegin());
 }
 
 WhitespaceManager::Change::Change(
     bool CreateReplacement, SourceRange OriginalWhitespaceRange,
     unsigned IndentLevel, int Spaces, unsigned StartOfTokenColumn,
     unsigned NewlinesBefore, StringRef PreviousLinePostfix,
     StringRef CurrentLinePrefix, tok::TokenKind Kind, bool ContinuesPPDirective,
-    bool IsStartOfDeclName)
+    bool IsStartOfDeclName, bool IsInsideToken)
     : CreateReplacement(CreateReplacement),
       OriginalWhitespaceRange(OriginalWhitespaceRange),
       StartOfTokenColumn(StartOfTokenColumn), NewlinesBefore(NewlinesBefore),
       PreviousLinePostfix(PreviousLinePostfix),
       CurrentLinePrefix(CurrentLinePrefix), Kind(Kind),
       ContinuesPPDirective(ContinuesPPDirective),
       IsStartOfDeclName(IsStartOfDeclName), IndentLevel(IndentLevel),
-      Spaces(Spaces), IsTrailingComment(false), TokenLength(0),
-      PreviousEndOfTokenColumn(0), EscapedNewlineColumn(0),
+      Spaces(Spaces), IsInsideToken(IsInsideToken), IsTrailingComment(false),
+      TokenLength(0), PreviousEndOfTokenColumn(0), EscapedNewlineColumn(0),
       StartOfBlockComment(nullptr), IndentationOffset(0) {}
 
 void WhitespaceManager::reset() {
   Changes.clear();
   Replaces.clear();
 }
 
 void WhitespaceManager::replaceWhitespace(FormatToken &Tok, unsigned Newlines,
                                           unsigned IndentLevel, unsigned Spaces,
                                           unsigned StartOfTokenColumn,
                                           bool InPPDirective) {
   if (Tok.Finalized)
     return;
   Tok.Decision = (Newlines > 0) ? FD_Break : FD_Continue;
   Changes.push_back(
       Change(/*CreateReplacement=*/true, Tok.WhitespaceRange, IndentLevel,
              Spaces, StartOfTokenColumn, Newlines, "", "", Tok.Tok.getKind(),
              InPPDirective && !Tok.IsFirst,
-             Tok.is(TT_StartOfName) || Tok.is(TT_FunctionDeclarationName)));
+             Tok.is(TT_StartOfName) || Tok.is(TT_FunctionDeclarationName),
+             /*IsInsideToken=*/false));
 }
 
 void WhitespaceManager::addUntouchableToken(const FormatToken &Tok,
                                             bool InPPDirective) {
   if (Tok.Finalized)
     return;
   Changes.push_back(Change(
       /*CreateReplacement=*/false, Tok.WhitespaceRange, /*IndentLevel=*/0,
       /*Spaces=*/0, Tok.OriginalColumn, Tok.NewlinesBefore, "", "",
       Tok.Tok.getKind(), InPPDirective && !Tok.IsFirst,
-      Tok.is(TT_StartOfName) || Tok.is(TT_FunctionDeclarationName)));
+      Tok.is(TT_StartOfName) || Tok.is(TT_FunctionDeclarationName),
+      /*IsInsideToken=*/false));
 }
 
 void WhitespaceManager::replaceWhitespaceInToken(
     const FormatToken &Tok, unsigned Offset, unsigned ReplaceChars,
     StringRef PreviousPostfix, StringRef CurrentPrefix, bool InPPDirective,
     unsigned Newlines, unsigned IndentLevel, int Spaces) {
   if (Tok.Finalized)
     return;
   SourceLocation Start = Tok.getStartOfNonWhitespace().getLocWithOffset(Offset);
   Changes.push_back(Change(
       true, SourceRange(Start, Start.getLocWithOffset(ReplaceChars)),
       IndentLevel, Spaces, std::max(0, Spaces), Newlines, PreviousPostfix,
-      CurrentPrefix,
-      // If we don't add a newline this change doesn't start a comment. Thus,
-      // when we align line comments, we don't need to treat this change as one.
-      // FIXME: We still need to take this change in account to properly
-      // calculate the new length of the comment and to calculate the changes
-      // for which to do the alignment when aligning comments.
-      Tok.is(TT_LineComment) && Newlines > 0 ? tok::comment : tok::unknown,
+      CurrentPrefix, Tok.is(TT_LineComment) ? tok::comment : tok::unknown,
       InPPDirective && !Tok.IsFirst,
-      Tok.is(TT_StartOfName) || Tok.is(TT_FunctionDeclarationName)));
+      Tok.is(TT_StartOfName) || Tok.is(TT_FunctionDeclarationName),
+      /*IsInsideToken=*/Newlines == 0));
 }
 
 const tooling::Replacements &WhitespaceManager::generateReplacements() {
   if (Changes.empty())
     return Replaces;
 
   std::sort(Changes.begin(), Changes.end(), Change::IsBeforeInFile(SourceMgr));
   calculateLineBreakInformation();
   alignConsecutiveDeclarations();
   alignConsecutiveAssignments();
   alignTrailingComments();
   alignEscapedNewlines();
   generateChanges();
 
   return Replaces;
 }
 
 void WhitespaceManager::calculateLineBreakInformation() {
   Changes[0].PreviousEndOfTokenColumn = 0;
+  Change *LastOutsideTokenChange = &Changes[0];
   for (unsigned i = 1, e = Changes.size(); i != e; ++i) {
     unsigned OriginalWhitespaceStart =
         SourceMgr.getFileOffset(Changes[i].OriginalWhitespaceRange.getBegin());
     unsigned PreviousOriginalWhitespaceEnd = SourceMgr.getFileOffset(
         Changes[i - 1].OriginalWhitespaceRange.getEnd());
     Changes[i - 1].TokenLength = OriginalWhitespaceStart -
                                  PreviousOriginalWhitespaceEnd +
                                  Changes[i].PreviousLinePostfix.size() +
                                  Changes[i - 1].CurrentLinePrefix.size();
 
+    // If there are multiple changes in this token, sum up all the changes until
+    // the end of the line.
+    if (Changes[i - 1].IsInsideToken)
+      LastOutsideTokenChange->TokenLength +=
+          Changes[i - 1].TokenLength + Changes[i - 1].Spaces;
+    else
+      LastOutsideTokenChange = &Changes[i - 1];
+
     Changes[i].PreviousEndOfTokenColumn =
         Changes[i - 1].StartOfTokenColumn + Changes[i - 1].TokenLength;
 
     Changes[i - 1].IsTrailingComment =
-        (Changes[i].NewlinesBefore > 0 || Changes[i].Kind == tok::eof) &&
+        (Changes[i].NewlinesBefore > 0 || Changes[i].Kind == tok::eof ||
+         (Changes[i].IsInsideToken && Changes[i].Kind == tok::comment)) &&
         Changes[i - 1].Kind == tok::comment;
   }
   // FIXME: The last token is currently not always an eof token; in those
   // cases, setting TokenLength of the last token to 0 is wrong.
   Changes.back().TokenLength = 0;
   Changes.back().IsTrailingComment = Changes.back().Kind == tok::comment;
 
   const WhitespaceManager::Change *LastBlockComment = nullptr;
   for (auto &Change : Changes) {
+    // Reset the IsTrailingComment flag for changes inside of trailing comments
+    // so they don't get realigned later.
+    if (Change.IsInsideToken)
+      Change.IsTrailingComment = false;
     Change.StartOfBlockComment = nullptr;
     Change.IndentationOffset = 0;
     if (Change.Kind == tok::comment) {
       LastBlockComment = &Change;
     } else if (Change.Kind == tok::unknown) {
       if ((Change.StartOfBlockComment = LastBlockComment))
         Change.IndentationOffset =
             Change.StartOfTokenColumn -

cfe/trunk/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsSingleLineComments) {
   EXPECT_EQ("void f() {\n"
             "  lineWith(); // comment\n"
             "  // at start\n"
             "}",
             format("void              f() {\n"
                    "  lineWith(); // comment\n"
                    "  // at start\n"
                    "}"));
+  EXPECT_EQ("int xy; // a\n"
+            "int z;  // b",
+            format("int xy;    // a\n"
+                   "int z;    //b"));
+  EXPECT_EQ("int xy; // a\n"
+            "int z; // bb",
+            format("int xy;    // a\n"
+                   "int z;    //bb",
+                   getLLVMStyleWithColumns(12)));
 
   verifyFormat("#define A                                                  \\\n"
                "  int i; /* iiiiiiiiiiiiiiiiiiiii */                       \\\n"
                "  int jjjjjjjjjjjjjjjjjjjjjjjj; /* */",
                getLLVMStyleWithColumns(60));
   verifyFormat(
       "#define A                                                   \\\n"
       "  int i;                        /* iiiiiiiiiiiiiiiiiiiii */ \\\n"