[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

105–119

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."

106

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 '\'.

110

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.

131–134

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

110

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.

131–134

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

[sammccall]

Still LG

clang/lib/Format/BreakableToken.cpp

110

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]?\\.");
+  // Some spaces are unacceptable to break on, rewind past them.
   while (SpaceOffset != StringRef::npos) {
+    // 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 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);
+        continue;
+      }
+    }
+
     // 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)))
+    if (kNumberedListRegexp.match(Text.substr(SpaceOffset).ltrim(Blanks))) {
       SpaceOffset = Text.find_last_of(Blanks, SpaceOffset);
+      continue;
+    }
+
     // Avoid ever breaking before a { in JavaScript.
-    else if (Style.Language == FormatStyle::LK_JavaScript &&
-             SpaceOffset + 1 < Text.size() && Text[SpaceOffset + 1] == '{')
+    if (Style.Language == FormatStyle::LK_JavaScript &&
+        SpaceOffset + 1 < Text.size() && Text[SpaceOffset + 1] == '{') {
       SpaceOffset = Text.find_last_of(Blanks, SpaceOffset);
-    else
-      break;
+      continue;
+    }
+
   }
 
   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)));
+    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 */",