[PATCH] D90949: [clang-format] avoid introducing multiline comments

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

One concern about looping, otherwise just style nits.



================
Comment at: clang/lib/Format/BreakableToken.cpp:104
   static const auto kNumberedListRegexp = llvm::Regex("^[1-9][0-9]?\\.");
   while (SpaceOffset != StringRef::npos) {
+    // In C++ with the -Werror=comment option, having multiline comments (
----------------
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."


================
Comment at: clang/lib/Format/BreakableToken.cpp:105
   while (SpaceOffset != StringRef::npos) {
+    // In C++ with the -Werror=comment option, having multiline comments (
+    // two consecutive comment lines, the first ending in `\`) is an
----------------
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 '\'.
```


================
Comment at: clang/lib/Format/BreakableToken.cpp:109
+    // after '\'.
+    if (Style.isCpp()) {
+      StringRef::size_type LastNonBlank =
----------------
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.


================
Comment at: clang/lib/Format/BreakableToken.cpp:125
     else
       break;
   }
----------------
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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90949/new/

https://reviews.llvm.org/D90949