JonasToth marked 15 inline comments as done.
JonasToth added inline comments.
================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:52
+ const LangOptions &LangOpts) {
+ assert(Indirections >= 0 && "Indirections must be non-negative");
+ if (Indirections == 0)
----------------
aaron.ballman wrote:
> This assertion suggests that `Indirections` should be `unsigned` and that you
> perform the assertion before doing a decrement to ensure it isn't trying to
> decrement past 0.
Because the decrement is post-fix it will decrement past 0 on the breaking
condition.
Having `unsigned` will result in a wrap (is defined, but still slightly
non-obvious).
I could make a `do - while`, then the condition can be `--Indirections != 0`. I
would just like to follow the CPPCG that say 'don't use unsigned unless you
need modulo arithmetic'.
================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:230
+ for (const auto &Range : Ranges) {
+ auto CharRange = Lexer::getAsCharRange(
+ CharSourceRange::getCharRange(Range.getBegin(), Range.getEnd()), SM,
----------------
aaron.ballman wrote:
> Use of `auto`?
I can use `CharSourceRange` too. kbobryev wanted `auto` :)
================
Comment at: clang-tidy/utils/LexerUtils.cpp:85
+ llvm::Optional<Token> Tok = Lexer::findNextToken(Loc, SM, LangOpts);
+ assert(Tok && "Could not retrieve next token");
+
----------------
aaron.ballman wrote:
> This seems like a bad assertion -- it's an optional for a reason, isn't it?
In the context of the check it is expected that this range is valid and all
tokens can be found. I adjusted the naming of the function to better reflect
the purpose.
================
Comment at: clang-tidy/utils/LexerUtils.h:48
+ Token T;
+ bool FailedRetrievingToken = Lexer::getRawToken(L, T, SM, LangOpts);
+
----------------
aaron.ballman wrote:
> No real need for this local.
I found it unexpected, that failure is signaled with `true`, so for readability
I added this longer tmp variable. Are there other ways to signal `true ==
failure`?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51949
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
