[PATCH] D51949: [clang-tidy] new check 'readability-isolate-declaration'

[Jonas Toth via Phabricator via cfe-commits]

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