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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits