[clang-format] avoid introducing multiline comments

[krasimir]

Summary

In C++ with -Werror=comment, multiline comments are not allowed.
clang-format could accidentally introduce multiline comments when reflowing.
This adapts clang-format to not introduce multiline comments by not allowing a
break after \. Note that this does not apply to comment lines that already are
multiline comments, such as comments in macros.

[sammccall]

One concern about looping, otherwise just style nits.

clang/lib/Format/BreakableToken.cpp

104

Can you add a high-level comment to this loop?

I guess the idea is "Some spaces are unacceptable to break on, rewind past them."

105

The warning is worth mentioning, but fundamentally we have the warning because such code is confusing, and we shouldn't produce confusing code.

maybe:

// If a line-comment ends with `\`, the next line continues the comment,
// whether or not it starts with `//`. This is confusing and triggers -Wcomment.
// Avoid introducing multiline comments by not allowing a break after '\'.

109

Do we really want to predicate this on isCpp()? // comments are allowed by C99.
Even if the warning only applies to C++ for some reason, the reasons for confusion do not.

125

doesn't this mean that we won't loop? if Text ends with "blah \ \" then you'll split between "blah" and the first "\"?

I guess this could be structured:

while () {
  if (special case 1) {
    // adjust pos
    continue;
  }
  if (special case 2) {
    // adjust pos
    continue;
  }
  break;
}

(This is equivalent to the old if/elseif/break which is too hard to add complex conditions to)

[krasimir]

• address review comments

[krasimir]

clang/lib/Format/BreakableToken.cpp

109

I think in Java and other non-C++-y languages, an \ at the end of a line-comment line does not have any special meaning, hence I didn't want it to trigger in those cases.

125

Thank you! Updated this block and added a test case for this.

[sammccall]

Still LG

clang/lib/Format/BreakableToken.cpp

109

Sigh, I guess I forgot what isCpp() means again... :)

clang/lib/Format/BreakableToken.cpp

if (SpaceOffset != StringRef::npos && SpaceOffset + 1 < Text.size() &&
         Text[SpaceOffset + 1] == '{') {
       MaxSplitBytes = SpaceOffset + 1;
     }
   }
 
   StringRef::size_type SpaceOffset = Text.find_last_of(Blanks, MaxSplitBytes);
 
   static const auto kNumberedListRegexp = llvm::Regex("^[1-9][0-9]?\\.");
   while (SpaceOffset != StringRef::npos) {
+    // In C++ with the -Werror=comment option, having multiline comments (
+    // two consecutive comment lines, the first ending in `\`) is an
+    // error. Avoid introducing multiline comments by not allowing a break right
+    // after '\'.
+    if (Style.isCpp()) {
+      StringRef::size_type LastNonBlank =
+          Text.find_last_not_of(Blanks, SpaceOffset);
+      if (LastNonBlank != StringRef::npos && Text[LastNonBlank] == '\\')
+        SpaceOffset = Text.find_last_of(Blanks, LastNonBlank);
+    }
+
     // Do not split before a number followed by a dot: this would be interpreted
     // as a numbered list, which would prevent re-flowing in subsequent passes.
     if (kNumberedListRegexp.match(Text.substr(SpaceOffset).ltrim(Blanks)))
       SpaceOffset = Text.find_last_of(Blanks, SpaceOffset);
     // Avoid ever breaking before a { in JavaScript.
     else if (Style.Language == FormatStyle::LK_JavaScript &&
              SpaceOffset + 1 < Text.size() && Text[SpaceOffset + 1] == '{')
       SpaceOffset = Text.find_last_of(Blanks, SpaceOffset);
     else
       break;
   }
 
   if (SpaceOffset == StringRef::npos ||
       // Don't break at leading whitespace.
       Text.find_last_not_of(Blanks, SpaceOffset) == StringRef::npos) {
     // Make sure that we don't break at leading whitespace that
     // reaches past MaxSplit.
     StringRef::size_type FirstNonWhitespace = Text.find_first_not_of(Blanks);

clang/unittests/Format/FormatTestComments.cpp

EXPECT_EQ("double\n"
             "          // AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\\\n"
             "          // AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA",
             format("double a; // AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\\\n"
                    "          // AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\\\n"
                    "          // AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA",
                    getLLVMStyleWithColumns(49)));
 }
 
+TEST_F(FormatTestComments, DontIntroduceMultilineComments) {
+  // Avoid introducing a multiline comment by breaking after `\`.
+  for (int ColumnLimit = 15; ColumnLimit <= 17; ++ColumnLimit) {
+    EXPECT_EQ(
+        "// aaaaaaaaaa\n"
+        "// \\ bb",
+        format("// aaaaaaaaaa \\ bb", getLLVMStyleWithColumns(ColumnLimit)));
+    EXPECT_EQ(
+        "// aaaaaaaaa\n"
+        "// \\  bb",
+        format("// aaaaaaaaa \\  bb", getLLVMStyleWithColumns(ColumnLimit)));
+  }
+}
+
 TEST_F(FormatTestComments, DontSplitLineCommentsWithPragmas) {
   FormatStyle Pragmas = getLLVMStyleWithColumns(30);
   Pragmas.CommentPragmas = "^ IWYU pragma:";
   EXPECT_EQ(
       "// IWYU pragma: aaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbb",
       format("// IWYU pragma: aaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbb", Pragmas));
   EXPECT_EQ(
       "/* IWYU pragma: aaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbb */",