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

[Jonas Toth via Phabricator via cfe-commits]

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:
> 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.
I totally see the point, and it started as `unsigned` as well, but it felt uncomfortable. thanks for letting me do it :)


================
Comment at: docs/clang-tidy/checks/readability-isolate-declaration.rst:3
+
+readability-isolate-declarationaration
+======================================
----------------
aaron.ballman wrote:
> Typo. declarationaration -> declaration
> 
> (Don't forget to fix the underlines as well.)
That was definitly too deep in the night :D


================
Comment at: docs/clang-tidy/checks/readability-isolate-declaration.rst:45
+
+Global variables and member variables are excluded.
+
----------------
aaron.ballman wrote:
> 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?
I thought so too, but take a look at the AST:

```
int global_i, global_j = 42, global_k;
void f() {
    int local_i, local_j = 42, local_k;
}
```

```
|-VarDecl 0x55c428c4ab08 </home/jonas/Programme/test/test_decls_global.cpp:1:1, col:5> col:5 global_i 'int'
|-VarDecl 0x55c428c4abc0 <col:1, col:26> col:15 global_j 'int' cinit
| `-IntegerLiteral 0x55c428c4ac20 <col:26> 'int' 42
|-VarDecl 0x55c428c4ac58 <col:1, col:30> col:30 global_k 'int'
`-FunctionDecl 0x55c428c4ad30 <line:2:1, line:4:1> line:2:6 f 'void ()'
  `-CompoundStmt 0x55c428c4af90 <col:10, line:4:1>
    `-DeclStmt 0x55c428c4af78 <line:3:5, col:39>
      |-VarDecl 0x55c428c4ade8 <col:5, col:9> col:9 local_i 'int'
      |-VarDecl 0x55c428c4ae60 <col:5, col:28> col:18 local_j 'int' cinit
      | `-IntegerLiteral 0x55c428c4aec0 <col:28> 'int' 42
      `-VarDecl 0x55c428c4aef8 <col:5, col:32> col:32 local_k 'int'
```

The locals create a `DeclStmt` and the globals don't, right now the transformation depends on the fact that its a `DeclStmt`, similar to class members that are `FieldDecl`.

I did short investigation on the issue, but couldn't think of a good way to fix that issue. I was not capable of "grouping" these decls even though they are together. Maybe I just missed something obvious, but right it's not supported. I would love to actually support it tough.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51949