Page MenuHomePhabricator

[clang-tidy] new check 'readability-isolate-declaration'
ClosedPublic

Authored by JonasToth on Sep 11 2018, 2:16 PM.

Details

Summary

This patch introduces a new clang-tidy check that matches on all declStmt that declare more then one variable
and transform them into one statement per declaration if possible.

It currently only focusses on variable declarations but should be extended to cover more kinds of declarations in the future.
It is related to https://reviews.llvm.org/D27621 and does use it's extensive test-suite. Thank you to firolino for his work!

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
aaron.ballman added inline comments.Sep 28 2018, 11:07 AM
clang-tidy/readability/IsolateDeclarationCheck.cpp
230 ↗(On Diff #167491)

Use of auto?

250 ↗(On Diff #167491)

Excepts -> Accepts

291 ↗(On Diff #167491)

Debugging code that can be removed.

clang-tidy/utils/LexerUtils.cpp
89

This seems like a bad assertion -- it's an optional for a reason, isn't it?

clang-tidy/utils/LexerUtils.h
49

No real need for this local.

How about creating CERT alias?

clang-tidy/readability/IsolateDeclarationCheck.cpp
58 ↗(On Diff #167491)

It'll be better to compare with 0 explicitly.

145 ↗(On Diff #167491)

auto could be used here.

198 ↗(On Diff #167491)

Will be good idea to use range loop.

docs/ReleaseNotes.rst
60

Do we really need this entry?

  • simplify matcher slighty, as initStmt in if/switch don't have a compundstmt as parent, add suggested tests
JonasToth marked 15 inline comments as done.Sep 29 2018, 4:31 AM
JonasToth added inline comments.
clang-tidy/readability/IsolateDeclarationCheck.cpp
52 ↗(On Diff #167491)

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'.

230 ↗(On Diff #167491)

I can use CharSourceRange too. kbobryev wanted auto :)

clang-tidy/utils/LexerUtils.cpp
89

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.

clang-tidy/utils/LexerUtils.h
49

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?

JonasToth updated this revision to Diff 167588.Sep 29 2018, 4:31 AM
JonasToth marked 2 inline comments as done.
  • address review comments
JonasToth marked 2 inline comments as done.Sep 29 2018, 4:37 AM
JonasToth updated this revision to Diff 167589.Sep 29 2018, 4:38 AM
  • fix spurious changes
JonasToth updated this revision to Diff 167591.Sep 29 2018, 4:59 AM
  • remove changes from lexerutils refactoring in other checks
JonasToth updated this revision to Diff 167696.Oct 1 2018, 3:56 AM
  • remove spurious change in fixithintutils
mgehre added a subscriber: mgehre.Oct 2 2018, 3:32 PM
JonasToth updated this revision to Diff 168091.Oct 3 2018, 3:18 AM
JonasToth marked 2 inline comments as done.
  • remove spurious local variable
kbobyrev added inline comments.Oct 7 2018, 8:05 AM
clang-tidy/readability/IsolateDeclarationCheck.cpp
24 ↗(On Diff #168091)

It would be shorter to use llvm::all_of(Node.decls(), ...);

30 ↗(On Diff #168091)

Maybe inline this variable?

JonasToth marked 2 inline comments as done.Oct 7 2018, 10:24 AM

@kbobyrev is it ok for you if I stick with the Optional<> style in the check?

JonasToth updated this revision to Diff 168598.Oct 7 2018, 10:24 AM
  • address review comments, simplifying code

@kbobyrev is it ok for you if I stick with the Optional<> style in the check?

Yes, I think it's fine. Thank you very much for working on the patch, I am indeed very excited about this check.

Unfortunately, I won't be able to review the code in the upcoming few weeks as I caught a cold and I'm trying to get better before the LLVM Meeting, so if there is anyone else interested in reviewing proposed changes please feel free to jump in.

Unfortunately, I won't be able to review the code in the upcoming few weeks as I caught a cold and I'm trying to get better before the LLVM Meeting, so if there is anyone else interested in reviewing proposed changes please feel free to jump in.

Thank you very much for the review so far! I think this patch is already close to finish so anyone can continue with the review :)

JonasToth updated this revision to Diff 169389.Oct 12 2018, 6:43 AM
  • fix headline in doc
lebedev.ri added inline comments.Oct 26 2018, 6:59 AM
test/clang-tidy/readability-isolate-declaration.cpp
233 ↗(On Diff #169389)

Comment is misleading. Transform == fixit, at least for me.
But they are not even diagnosed.
So maybe

// TODO: Handle FieldDecl's as well

Mostly minor nits that clean up wording and comments.

clang-tidy/readability/IsolateDeclarationCheck.cpp
69 ↗(On Diff #169389)

No need for the isa<> check -- isFunctionPointerType() already does the right thing.

71 ↗(On Diff #169389)

You don't need to call getTypePtr() -- QualType overloads operator->() to do this directly.

141 ↗(On Diff #169389)

Member pointer -> Member pointers

191 ↗(On Diff #169389)

Member pointer -> Member pointers

52 ↗(On Diff #167491)

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.

clang-tidy/utils/LexerUtils.cpp
90

Tok->is()

clang-tidy/utils/LexerUtils.h
78

Relex the provide -> Re-lex the provided
spanning -> spans

81

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()

docs/clang-tidy/checks/readability-isolate-declaration.rst
3 ↗(On Diff #169389)

Typo. declarationaration -> declaration

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

10 ↗(On Diff #169389)

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;.

23 ↗(On Diff #169389)

does exclude -> excludes
commong -> common

31 ↗(On Diff #169389)

undesired in it's own -> undesirable

45 ↗(On Diff #169389)

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?

63 ↗(On Diff #169389)

Add a comma after "Furthermore".

JonasToth updated this revision to Diff 171346.Oct 26 2018, 1:59 PM
JonasToth marked 16 inline comments as done.
  • address review comments
clang-tidy/readability/IsolateDeclarationCheck.cpp
52 ↗(On Diff #167491)

I totally see the point, and it started as unsigned as well, but it felt uncomfortable. thanks for letting me do it :)

docs/clang-tidy/checks/readability-isolate-declaration.rst
3 ↗(On Diff #169389)

That was definitly too deep in the night :D

45 ↗(On Diff #169389)

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.

JonasToth marked 2 inline comments as done.Oct 26 2018, 1:59 PM
JonasToth updated this revision to Diff 171361.Oct 26 2018, 3:08 PM
  • [Fix] wrong condition in matcher coming from incorrect code change
Eugene.Zelenko added inline comments.Oct 26 2018, 3:14 PM
docs/ReleaseNotes.rst
106

May be Finds or Detects like other checks? Same in documentation.

JonasToth marked an inline comment as done.Oct 29 2018, 10:54 AM
  • Merge branch 'master' into experiment_isolate_decl
  • adjust doc slighty to comment
ZaMaZaN4iK added inline comments.
clang-tidy/readability/IsolateDeclarationCheck.cpp
119 ↗(On Diff #171538)

Mark DeclCount as const

This is usually not done in the LLVM codebase, as its not a
pointer/reference.

Am 29.10.18 um 20:29 schrieb Alexander Zaitsev via Phabricator:

ZaMaZaN4iK added inline comments.

================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:119
+ const LangOptions &LangOpts) {
+ std::size_t DeclCount = std::distance(DS->decl_begin(), DS->decl_end());
+ if (DeclCount < 2)


Mark DeclCount as const

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D51949

JonasToth marked an inline comment as done.Oct 31 2018, 6:48 AM
JonasToth added inline comments.
clang-tidy/readability/IsolateDeclarationCheck.cpp
119 ↗(On Diff #171538)

as DeclCount is a value, LLVM style does not mark it as const, only pointers or references.

JonasToth marked 2 inline comments as done.Oct 31 2018, 6:49 AM
aaron.ballman accepted this revision.Oct 31 2018, 7:25 AM

LGTM!

docs/clang-tidy/checks/readability-isolate-declaration.rst
45 ↗(On Diff #169389)

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.

This revision is now accepted and ready to land.Oct 31 2018, 7:25 AM
JonasToth marked 3 inline comments as done.Oct 31 2018, 9:34 AM
JonasToth updated this revision to Diff 171947.Oct 31 2018, 9:47 AM
  • Merge branch 'master' into experiment_isolate_decl
  • remove unused matcher
This revision was automatically updated to reflect the committed changes.