[clang-format] Fix alignment of preprocessor trailing comments

[krasimir]

Summary

This patch is a follow-up of https://reviews.llvm.org/rL304687, which fixed an
overflow in the comment alignment code in clang-format. The token length of
trailing comments of preprocessor directives is calculated incorrectly by
including the text between consecutive directives. That causes them to not being
aligned.

For example, in this code with column limit 20

#if A
#else  // A
int iiii;
#endif // B

the length of the token // A was wrongly calculated as 14 = 5 (the size of // A\n) plus 9 (the size of int iiii;) and so // A wouldn't be aligned with // B and this was produced:

#if A
#else // A
int iiii;
#endif // B

This patch fixes this case.

[alexfh]

LG

lib/Format/WhitespaceManager.cpp

112

I'd pull SourceMgr.getCharacterData(PreviousOriginalWhitespaceEnd) out to a variable.

[krasimir]

• Address review comments

lib/Format/WhitespaceManager.cpp

const tooling::Replacements &WhitespaceManager::generateReplacements() {
 
   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 +
+    SourceLocation OriginalWhitespaceStart =
+        Changes[i].OriginalWhitespaceRange.getBegin();
+    SourceLocation PreviousOriginalWhitespaceEnd =
+        Changes[i - 1].OriginalWhitespaceRange.getEnd();
+    unsigned OriginalWhitespaceStartOffset =
+        SourceMgr.getFileOffset(OriginalWhitespaceStart);
+    unsigned PreviousOriginalWhitespaceEndOffset =
+        SourceMgr.getFileOffset(PreviousOriginalWhitespaceEnd);
+    assert(PreviousOriginalWhitespaceEndOffset <=
+           OriginalWhitespaceStartOffset);
+    const char *const PreviousOriginalWhitespaceEndData =
+        SourceMgr.getCharacterData(PreviousOriginalWhitespaceEnd);
+    StringRef Text(PreviousOriginalWhitespaceEndData,
+                   SourceMgr.getCharacterData(OriginalWhitespaceStart) -
+                       PreviousOriginalWhitespaceEndData);
+    // Usually consecutive changes would occur in consecutive tokens. This is
+    // not the case however when analyzing some preprocessor runs of the
+    // annotated lines. For example, in this code:
+    //
+    // #if A // line 1
+    // int i = 1;
+    // #else B // line 2
+    // int i = 2;
+    // #endif // line 3
+    //
+    // one of the runs will produce the sequence of lines marked with line 1, 2
+    // and 3. So the two consecutive whitespace changes just before '// line 2'
+    // and before '#endif // line 3' span multiple lines and tokens:
+    //
+    // #else B{change X}[// line 2
+    // int i = 2;
+    // ]{change Y}#endif // line 3
+    //
+    // For this reason, if the text between consecutive changes spans multiple
+    // newlines, the token length must be adjusted to the end of the original
+    // line of the token.
+    auto NewlinePos = Text.find_first_of('\n');
+    if (NewlinePos == StringRef::npos) {
+      Changes[i - 1].TokenLength = OriginalWhitespaceStartOffset -
+                                   PreviousOriginalWhitespaceEndOffset +
                                    Changes[i].PreviousLinePostfix.size() +
                                    Changes[i - 1].CurrentLinePrefix.size();
+    } else {
+      Changes[i - 1].TokenLength =
+          NewlinePos + 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 && Changes[i - 1].NewlinesBefore == 0)
       LastOutsideTokenChange->TokenLength +=
           Changes[i - 1].TokenLength + Changes[i - 1].Spaces;
     else
       LastOutsideTokenChange = &Changes[i - 1];

unittests/Format/FormatTestComments.cpp

verifyFormat("int f() {\n"
                "  i = 2;\n"
                "#else // comment about #else\n"
                "  // section comment 3\n"
                "  i = 4;\n"
                "#endif\n"
                "}", getLLVMStyleWithColumns(80));
 }
 
+TEST_F(FormatTestComments, AlignsPPElseEndifComments) {
+  verifyFormat("#if A\n"
+               "#else  // A\n"
+               "int iiii;\n"
+               "#endif // B",
+               getLLVMStyleWithColumns(20));
+  verifyFormat("#if A\n"
+               "#else  // A\n"
+               "int iiii; // CC\n"
+               "#endif // B",
+               getLLVMStyleWithColumns(20));
+  EXPECT_EQ("#if A\n"
+            "#else  // A1\n"
+            "       // A2\n"
+            "int ii;\n"
+            "#endif // B",
+            format("#if A\n"
+                   "#else  // A1\n"
+                   "       // A2\n"
+                   "int ii;\n"
+                   "#endif // B",
+                   getLLVMStyleWithColumns(20)));
+}
+
 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"