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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
