sammccall added a comment.
Mostly looks good. There's a bunch of ifs that are now trivial (condition and
body each fit on one line, no else) and we'd usually drop braces there, but
it's probably not worth the hassle.
I have to say that this rule slightly improves "early exit after error
condition" type code, but makes it less clear that "there are 3 cases" type
code is exhaustive.
So personally I'm not sure the check is a win if we want to follow it
everywhere, I'd consider fixing the cases where it improves things and then
turning it off. @kadircet am I being too picky here?
================
Comment at: clang-tools-extra/clangd/JSONTransport.cpp:260
continue;
} else {
// An empty line indicates the end of headers.
----------------
This seems inconsistent: we're elseing after continue.
If we've using this style, i'd invert the if here so the "continue" case just
falls off the end of the loop body. (But keep the comment)
================
Comment at: clang-tools-extra/clangd/JSONTransport.cpp:283
+ Read = retryAfterSignalUnlessShutdown(
+ 0, [&] { return std::fread(&JSON[Pos], 1, ContentLength - Pos, In); });
if (Read == 0) {
----------------
Looks like there are unrelated formatting changes in a couple of files
================
Comment at: clang-tools-extra/clangd/Selection.cpp:350
}
+ /* fall through and treat as part of the macro body */
}
----------------
FWIW I find this one *much* less clear :-(
I'd personally prefer to ignore the rule in this case, but curious what others
think
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113892/new/
https://reviews.llvm.org/D113892
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
