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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Mostly minor nits that clean up wording and comments.



================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:52
+                                             const LangOptions &LangOpts) {
+  assert(Indirections >= 0 && "Indirections must be non-negative");
+  if (Indirections == 0)
----------------
JonasToth wrote:
> 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'.
I disagree with this logic (personally, I think this is a code smell), but don't feel strongly enough about it to ask you to change it.


================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:69
+static int countIndirections(const Type *T, int Indirections = 0) {
+  if (isa<PointerType>(T) && T->isFunctionPointerType()) {
+    const auto *Pointee =
----------------
No need for the `isa<>` check -- `isFunctionPointerType()` already does the right thing.


================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:71
+    const auto *Pointee =
+        T->getPointeeType().getTypePtr()->castAs<FunctionType>();
+    return countIndirections(
----------------
You don't need to call `getTypePtr()` -- `QualType` overloads `operator->()` to do this directly.


================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:141
+
+  // FIXME: Member pointer are not transformed correctly right now, that's
+  // why they are treated as problematic here.
----------------
Member pointer -> Member pointers


================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:191
+
+    // FIXME: Member pointer are not transformed correctly right now, that's
+    // why they are treated as problematic here.
----------------
Member pointer -> Member pointers


================
Comment at: clang-tidy/utils/LexerUtils.cpp:86
+
+    if ((*Tok).is(tok::hash))
+      return false;
----------------
`Tok->is()`


================
Comment at: clang-tidy/utils/LexerUtils.h:78
+
+/// Relex the provide \p Range and return \c false if either a macro spanning
+/// multiple tokens, a pre-processor directive or failure to retrieve the
----------------
Relex the provide -> Re-lex the provided
spanning -> spans


================
Comment at: clang-tidy/utils/LexerUtils.h:81
+/// next token is found, otherwise \c true.
+bool saveFromPreProcessor(SourceRange Range, const SourceManager &SM,
+                          const LangOptions &LangOpts);
----------------
I don't think the functionality is obvious from the name -- this is testing to see whether any token in the given range is either a macro or a preprocessor directive. How about reversing the logic to: `rangeContainsExpansionsOrDirectives()`


================
Comment at: docs/clang-tidy/checks/readability-isolate-declaration.rst:3
+
+readability-isolate-declarationaration
+======================================
----------------
Typo. declarationaration -> declaration

(Don't forget to fix the underlines as well.)


================
Comment at: docs/clang-tidy/checks/readability-isolate-declaration.rst:10
+The automatic code-transformation will use the same indentation as the original
+for every created statement and add a linebreak after each statement.
+
----------------
linebreak -> line break

I think it may also be important to point out that the declarations will remain in the same order as their original source order. For instance, you may run into code like: `int i = 5, j = i;` and it's crucial that it be transformed into `int i = 5; int j = i;`.


================
Comment at: docs/clang-tidy/checks/readability-isolate-declaration.rst:23
+
+The check does exclude places where it is necessary or commong to declare
+multiple variables in one statement and there is no other way supported in the
----------------
does exclude -> excludes
commong -> common


================
Comment at: docs/clang-tidy/checks/readability-isolate-declaration.rst:31
+  // before the loop will increase the scope of the variable 'Begin' and 'End'
+  // which is undesired in it's own.
+  for (int Begin = 0, End = 100; Begin < End; ++Begin);
----------------
undesired in it's own -> undesirable


================
Comment at: docs/clang-tidy/checks/readability-isolate-declaration.rst:45
+
+Global variables and member variables are excluded.
+
----------------
Why are global variables excluded from this check? It seems like they should have the exact same behavior as local variables in terms of the transformation, no?


================
Comment at: docs/clang-tidy/checks/readability-isolate-declaration.rst:63
+
+Furthermore the transformation is very cautious when it detects various kinds
+of macros or preprocessor directives in the range of the statement. In this
----------------
Add a comma after "Furthermore".


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51949