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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!



================
Comment at: docs/clang-tidy/checks/readability-isolate-declaration.rst:45
+
+Global variables and member variables are excluded.
+
----------------
JonasToth wrote:
> 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.
Yeah, this looks like a deficiency with the AST representation, to me. It doesn't need to be solved for this patch, but it may be an interesting next step for the check for a future patch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51949