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

[JonasToth]

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!

[kbobyrev]

clang-tidy/readability/IsolateDeclCheck.cpp

324

llvm::StringRef::rtrim()?

Also, naming str should be at least Str, it1 -> It, ch -> Ch (or better Symbol) ...

332

Use llvm::join with ";\n" + Indent as the Separator?

334

Range?

[JonasToth]

• move lexer stuff into LexerUtils
• remove custom string handling function
• address review comments

clang-tidy/readability/IsolateDeclCheck.cpp

22

No, they will be removed, just for the current prototyping. using llvm::outs and llvm::errs would have been better though ;)

324

rtrim() is a better fit, but introduces another copy. Do you think I should rater use llvm::SmallString?

332

Yup, didn't know that one yet, thanks :)

344

s/hurts/reduces/? hurts sound a bit weird i think.

Lebedev wanted the number of decls in the diagnostic, would you include it or rather now?

[JonasToth]

• add space after clang-tidy in banner
• refactor slightly, add better in-code comments to explain whats happening, add more TODOs and FIXMEs

[kbobyrev]

A lot of good improvements and many test cases, thank you!

The comments are mostly nits.

clang-tidy/readability/IsolateDeclCheck.cpp

40

nit: return InitStmt ? InnerMatcher... : false;

66

i-> I

or even better while (--Indirections) since it's not used anywhere afterwards.

69

Also, I don't think this should happen with the correct behavior. llvm::Expected<T> looks like a better alternative for error handling here.

79

nit: "Type has to be set for *countIndirections()*"? The wording is not optimal, but currently the assertion message doesn't say much about the error.

94

just return (Condition0 || Condition1 || Condition2);

103

nit: naming (lowercase + more descriptive name/documentation about the returned value).

167

This assertion message doesn't really give much information, too. Before you inspect the code closely (e.g. to understand what "here" refers to), it would be hard for other developers to understand what the issue might just by looking at the failed assertion message unless they are very familiar with the code.

169

nit FIXME:. Also, ideally there would be an XFAIL test on that.

174

nit: const SourceLocation DeclEnd = findNextTerimator(CurrentDecl->hasInit() ? CurrentDecl->getINit()->getEndLoc() : CurrentDecl->getEndLoc(), SM, LagOpts); (formatted with Clang-Format, of course, my snippet is messy :)

Up to you, though, I just think it would be slightly more readable.

209

nit: const std::vector<T> & -> llvm::ArrayRef<T>

Also, probably SourceFromRanges -> collectSourceText(Ranges, ...)? This would probably be easier to read (otherwise it's easier to confuse with something like "get SourceLocations from SourceRanges when not looking at the function body).

212

nit: It would be probably easier to have Snippets(Ranges.size() and then insert each Snippet to the correct position. That would require sacrificing for-range loop and having a counter, but I think that it would make this piece slightly better. Up to you, I don't have strong opinion about this part.

215

probably auto CharRange = ...? The type is more or explicitly typed in the right-hand side expression and for seeing what CR actually is in the code below it's easier to have more descriptive variable name rather than explicit type in the declaration LHS (if I understand the trade-off correctly here).

219

(I have another comment about llvm::Optional vs llvm::Expected below, more information there)

Many of these checks actually print something to std::cerr and they look like actual errors (e.g. I don't think CharSourceRanges should be invalid in some scenario), so I think these should be treated like errors.

245

Same here (llvm::ArrayRef)

Also, functions should start with lowercase (i.e. -> createIsolatedDecls).

250

Same here, please use I instead of i. Also, Snippets is just a vector, therefore having I < Snippets.size() wouldn't be expensive (which is usually the reason behind storing End iterator in the cycle).

256

Maybe just inline it? This is only one line and it is only used once.

269

nit: auto here? Since you're not actually using the variable a lot and just pass to other functions, it might be easier to read if the type was deduced autmatically.

274

nit: llvm::ArrayRef or just inline it in the function call next line (is probably better).

Same below.

279

Both for PotentialSlices and PotentialSnippets: is there any case where any can not be determined and this is not an error? They both are llvm::Optional<T>s and the check(Result) just returns if any of them are not set, is there any case when any of the variables is actually not set, but this is the correct behavior? If not, IMO it would be better to use llvm::Expected: then, if the check fails, either print .takeError() message or propagate it "upstairs" (although I'm not really sure what would be better here).

344

"decreases" is also fine. "hurts" is probably too strong, I agree.

Up to you. Personally, I don't see any value in having the diagnostic message saying "hey, you have 2 declarations within one statement, that's really bad!" or "hey, you have 5 declarations within one statement..." - in both cases the point is that there are *multiple* declarations. I also don't think it would make debugging easier because you also check the formatting, so you already imply that the correct number of declarations was detected.

I'm interested to know what @lebedev.ri thinks.

test/clang-tidy/readability-isolate-decl.cpp

143

nit FIXME (and also XFAIL test if you have enough time, it's fine to leave FIXMEs in general, though.

[kbobyrev]

Also, regarding error handling and llvm::Option vs llvm::Expected: I think the case where the check most likely wouldn't be able to provide useful diagnostics and perform enough analysis is when there are macro expansions within inspected statement SourceRange. It might make sense to completely disable it in this case both because it's hard to do anything about the range transformation and because it seems to be hard to analyze this case. E.g. if the whole statement is expanded from a single macro and then the check would report on every macro usage (while the actual problem is in the macro itself). I don't know whether the check should support macros at all, it might make sense to mention this in the documentation and add few tests if we decide to go this way.

[lebedev.ri]

clang-tidy/readability/IsolateDeclCheck.cpp

344

I'm interested to know what @lebedev.ri thinks.

"This translation unit has an error. Can not continue" is also a diagnostic message.
Why are we not ok with that one, and want compiler to be a bit more specific?

Similarly here, why just point out that this code is bad as per the check,
without giving a little bit more info, that you already have?

[aaron.ballman]

clang-tidy/readability/IsolateDeclCheck.cpp

344

"This translation unit has an error. Can not continue" is also a diagnostic message.
Why are we not ok with that one, and want compiler to be a bit more specific?

Similarly here, why just point out that this code is bad as per the check, without giving a little bit more info, that you already have?

More information doesn't always equate into more understanding, especially when that information causes a distraction. For instance, you could argue that the type of the declared variables is also information we already have, but what purpose would it serve to tell it to the user?

Can you give an example where the specific number of declarations involved would help you to correct the diagnostic? I can't come up with one, so it feels to me like having the count is more of a distraction; especially given that there's no configurable threshold for "now you have too many declarations". I'd feel differently if there was a config option, because then the count is truly useful to know.

[kbobyrev]

clang-tidy/readability/IsolateDeclCheck.cpp

344

Oh, but that's different: "This translation unit has an error. Can not continue" does not provide enough information for users to fix the issue, pointing out that there are *multiple* declarations per statement is definitely enough.

[JonasToth]

There are testcases with macro around line 100 in the default tests. I
am not sure yet if #define I_DECLS int i1, i2, i3; should be
diagnosed, but the other cases should.

Am 27.09.2018 um 16:28 schrieb Kirill Bobyrev via Phabricator:

kbobyrev added a comment.

Also, regarding error handling and llvm::Option vs llvm::Expected: I think the case where the check most likely wouldn't be able to provide useful diagnostics and perform enough analysis is when there are macro expansions within inspected statement SourceRange. It might make sense to completely disable it in this case both because it's hard to do anything about the range transformation and because it seems to be hard to analyze this case. E.g. if the whole statement is expanded from a single macro and then the check would report on every macro usage (while the actual problem is in the macro itself). I don't know whether the check should support macros at all, it might make sense to mention this in the documentation and add few tests if we decide to go this way.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D51949

[JonasToth]

clang-tidy/readability/IsolateDeclCheck.cpp

344

I am personally against having the number in the diagnostic as well, it would only add value if the declarations are expanded from a macro.

@aaron.ballman Configuration of this check would be intersting but i would rather postpone that and have a basic working check first. Given that this aims to be utility-like to evaluate const-correctness and/or to be usable with other checks doing type transformations.

[aaron.ballman]

clang-tidy/readability/IsolateDeclCheck.cpp

344

Yeah, I wasn't suggesting a threshold config option for this patch so much as pointing out why I'm opposed to putting the count in the diagnostic.

[JonasToth]

clang-tidy/readability/IsolateDeclCheck.cpp

69

It currently happens for typedef int * IntPtr; IntPtr p;. Regarding Expected<T> see other comment.

279

Currently the Optional and checking for invalid and so on everywhere is result of the WIP and the bugs I encountered and try to fix. In general the operations, like re-lexing can fail judging from the interface, same for retrieving SourceLocations, so I took the very conservative approach to check everything.
In my opinion it is more user-friendly to just emit the warning, but without transformation instead of an error that the transformation fails for some reason. Running this check over multiple bigger code-bases will give better data to make an actual judgement on this.

In general there are code-constructs where transformation _could_ be dangerous or unexpted, all I can think of includes macros.

int SomeConfig = 42,
#if BUILD_WITH_X
     AnotherConfig = 43;
#else 
    AnotherConfig = 44;
#endif

This example is not unreasonable and currently transformed incorrectly. I wanted to have an mechanism to just bail if something is fishy. I tried llvm::Expected but thought that using llvm::Optional was just simpler for prototyping.
Semantically Expected would fit better, because the transformation is actually expected to work. Emitting notes that contain information from the errors might be user-friendly, but having a note: Can not automatically transform this declaration statement is probably similarly informative.
For now I'd stick with Optional and it might be even the better fit in general, considering that more declaration kinds should be implemented in the future (similar to the other, currently staled check).

[JonasToth]

• address review comments, most nits solved
• fix typedefs and function pointers with comments as distraction
• make memberpointer detection more accurate
• move functioning member pointer test
• clean debug output
• clean lexer utils as well
• adjust diagnostic message

[JonasToth]

• simplification on FIXME:/TODO:

clang-tidy/readability/IsolateDeclCheck.cpp

344

I used a different wording without the variable count. Adding configuration, more advanced diagnostic can be done in a follow up or in a different check, e.g. readability-function-size, as it does counting (might not be the perfect fit though)

[JonasToth]

clang-tidy/readability/IsolateDeclCheck.cpp

212

I prefer this one :)

[JonasToth]

• bail out in transformation if there are PP directives in the source range of the DeclStmt

[JonasToth]

• fix doc typo

[JonasToth]

• last cleanup for today, testing on clang/lib looks good, but is already very clean

[JonasToth]

• write user-facing documentation

[lebedev.ri]

I have looked through tests, and it is possible that i just missed it, but does it test/handle the cases like:

struct S {
  int X, Y, Z;  // <- should be diagnosed.
}

clang-tidy/readability/IsolateDeclCheck.cpp

344

I, too, wasn't suggesting a config option.
It really is about not having more than one variable per declaration, not more than N variables per declaration.
I would personally prefer to see the number (since it complains about the count), but it will be possible to live without it.

[JonasToth]

In D51949#1248850, @lebedev.ri wrote:

I have looked through tests, and it is possible that i just missed it, but does it test/handle the cases like:

struct S {
  int X, Y, Z;  // <- should be diagnosed.
}

Unfortunatly not :( The reason is, that int X, Y, Z; is not a DeclStmt but they are all FieldDecl (see https://godbolt.org/z/SXbQCa) which is why I excluded them for now.
I did not look further into the issue, it could be easy to fix, but I don't know.

[JonasToth]

clang-tidy/readability/IsolateDeclCheck.cpp

344

Yes, I understand the point, and it might even make sense to refactor that check later on.
ATM this is a workhorse for the const-correctness check refactorings, as they need isolated variable declarations ;)

[lebedev.ri]

In D51949#1248861, @JonasToth wrote:

In D51949#1248850, @lebedev.ri wrote:

I have looked through tests, and it is possible that i just missed it, but does it test/handle the cases like:

struct S {
  int X, Y, Z;  // <- should be diagnosed.
}

Unfortunatly not :( The reason is, that int X, Y, Z; is not a DeclStmt but they are all FieldDecl (see https://godbolt.org/z/SXbQCa) which is why I excluded them for now.
I did not look further into the issue, it could be easy to fix, but I don't know.

Negative tests are great to have too :)

[JonasToth]

test/clang-tidy/readability-isolate-decl.cpp

185

@lebedev.ri :P

[JonasToth]

• add tests for auto and decltype
• rename check to 'readability-isolate-declaration'

[lebedev.ri]

Thank you for working on this!

Tried on a pet project,
Two hits:

$ ninja
[16/376] Checking validity of cameras.xml
/home/lebedevri/rawspeed/data/cameras.xml validates
[100/376] Building CXX object src/CMakeFiles/rawspeed.dir/librawspeed/decoders/NefDecoder.cpp.o
/home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decoders/NefDecoder.cpp:700:3: warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
  double g[6], bnd[2]={0,0}, r;
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[127/376] Building CXX object fuzz/librawspeed/decompressors/HuffmanTable/CMakeFiles/HuffmanTableFuzzer-LUTVsLookup-BitPumpJPEG-NoFullDecode.dir/Dual.cpp.o
/home/lebedevri/rawspeed/build-Clang-SANITIZE/../fuzz/librawspeed/decompressors/HuffmanTable/Dual.cpp:99:7: warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
      int decoded0, decoded1;
      ^~~~~~~~~~~~~~~~~~~~~~~

clang-tidy-applied fix-its:

diff --git a/fuzz/librawspeed/decompressors/HuffmanTable/Dual.cpp b/fuzz/librawspeed/decompressors/HuffmanTable/Dual.cpp
index b1f86a2a..af5b9164 100644
--- a/fuzz/librawspeed/decompressors/HuffmanTable/Dual.cpp
+++ b/fuzz/librawspeed/decompressors/HuffmanTable/Dual.cpp
@@ -96,7 +96,8 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* Data, size_t Size) {
     rawspeed::PUMP bits1(bs1);
 
     while (true) {
-      int decoded0, decoded1;
+      int decoded0;
+      int decoded1;
 
       try {
         decoded0 = ht0.decode<decltype(bits0), FULLDECODE>(bits0);
diff --git a/src/librawspeed/decoders/NefDecoder.cpp b/src/librawspeed/decoders/NefDecoder.cpp
index c74d07a1..e0f841f8 100644
--- a/src/librawspeed/decoders/NefDecoder.cpp
+++ b/src/librawspeed/decoders/NefDecoder.cpp
@@ -697,7 +697,9 @@ std::vector<ushort16> NefDecoder::gammaCurve(double pwr, double ts, int mode,
   std::vector<ushort16> curve(65536);
 
   int i;
-  double g[6], bnd[2]={0,0}, r;
+  double g[6];
+  double bnd[2]={0,0};
+  double r;
   g[0] = pwr;
   g[1] = ts;
   g[2] = g[3] = g[4] = 0;

The fixes are correct.

There are no obvious false-positives on that codebase, e.g. sonarcloud finds one extra issue with struct fields, but no new issues that should have been caught by this check in it's current form.

So while i have not participated in the code review, this looks good to me.

[JonasToth]

LLVM works, Blender doesn't, the offending piece is float (((*f_ptr2)))[42], *f_ptr3, f_value2 = 42.f; (parens around the pointer). I am trying to fix that.

[JonasToth]

Blender looks better now, code compiles after full transformation

blender_isolated_decls.patch2 MBDownload

[JonasToth]

llvm and clang look fine as well. No miscompilation after fixing, tests run as well. (i do exclude all targets except x86 for build-speed)

cfe_isolated_decl.patch383 KBDownload

llvm_isolated_decl.patch725 KBDownload

From my side the functionality is complete. Thank you for the review and feedback so far!

[JonasToth]

• fix paren issue, introduced non-local changes i will extract and post in a separate patch
• remove debug output for paren search

[JonasToth]

docs/clang-tidy/checks/readability-isolate-declaration.rst

33 ↗

(On Diff #167491)

typo
mention if () int i, j, k = function(); as well

87 ↗

(On Diff #167491)

reduce indendation for PP directives

[aaron.ballman]

Can you add some tests for cases like:

// C code
void (*signal(int sig, void (*func)(int)))(int); // One decl
int i = sizeof(struct S { int i;}); // One decl
int j = sizeof(struct T { int i;}), k; // Two decls
void g(struct U { int i; } s); // One decl
void h(struct V { int i; } s), m(int i, ...); // Two decls

and

// C++ code
void f() {
  switch (int i = 12, j = 14; i) {} // Two decls, but don't transform
}

void g() try {
  int i, j; // Two decls
} catch (...) {}

clang-tidy/readability/IsolateDeclarationCheck.cpp

24 ↗

(On Diff #167491)

The identifier reads strangely to my ears. How about: onlyDeclaresVariables()?

52 ↗

(On Diff #167491)

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.

76–78 ↗

(On Diff #167491)

Use castAs<FunctionType>() and remove the assert.

83–84 ↗

(On Diff #167491)

if (const auto *AT = dyn_cast<ArrayType>(T)) rather than isa followed by cast.

150 ↗

(On Diff #167491)

Memberpointer -> Member pointer
thats -> that's

(Same elsewhere)

199–200 ↗

(On Diff #167491)

Use cast<> instead of dyn_cast and an assert.

[aaron.ballman]

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

85 ↗

(On Diff #167491)

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

clang-tidy/utils/LexerUtils.h

48 ↗

(On Diff #167491)

No real need for this local.

[Eugene.Zelenko]

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?

[JonasToth]

• simplify matcher slighty, as initStmt in if/switch don't have a compundstmt as parent, add suggested tests

[JonasToth]

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

85 ↗

(On Diff #167491)

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

48 ↗

(On Diff #167491)

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]

• address review comments

[JonasToth]

• fix spurious changes

[JonasToth]

• remove changes from lexerutils refactoring in other checks

[JonasToth]

• remove spurious change in fixithintutils

[JonasToth]

• remove spurious local variable

[kbobyrev]

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]

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

[JonasToth]

• address review comments, simplifying code

[kbobyrev]

In D51949#1257430, @JonasToth wrote:

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

[JonasToth]

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]

• fix headline in doc

[JonasToth]

ping :)

[lebedev.ri]

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

[aaron.ballman]

Mostly minor nits that clean up wording and comments.

clang-tidy/readability/IsolateDeclarationCheck.cpp

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.

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

clang-tidy/utils/LexerUtils.cpp

86 ↗

(On Diff #169389)

Tok->is()

clang-tidy/utils/LexerUtils.h

78 ↗

(On Diff #169389)

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

81 ↗

(On Diff #169389)

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]

• 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]

• [Fix] wrong condition in matcher coming from incorrect code change

[Eugene.Zelenko]

docs/ReleaseNotes.rst

99

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

[JonasToth]

• Merge branch 'master' into experiment_isolate_decl
• adjust doc slighty to comment

[ZaMaZaN4iK]

clang-tidy/readability/IsolateDeclarationCheck.cpp

119 ↗

(On Diff #171538)

Mark DeclCount as const

[JonasToth]

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]

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.

[aaron.ballman]

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.

[JonasToth]

• Merge branch 'master' into experiment_isolate_decl
• remove unused matcher

clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ContainerSizeEmptyCheck.cpp
   DeleteNullPointerCheck.cpp
   DeletedDefaultCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolConversionCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
+  IsolateDeclCheck.cpp
   MagicNumbersCheck.cpp
   MisleadingIndentationCheck.cpp
   MisplacedArrayIndexCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   NonConstParameterCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp

clang-tidy/readability/IsolateDeclCheck.h

+//===--- IsolateDeclCheck.h - clang-tidy-------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ISOLATEDECLCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ISOLATEDECLCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// FIXME: Write a short description.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-isolate-decl.html
+class IsolateDeclCheck : public ClangTidyCheck {
+public:
+  IsolateDeclCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ISOLATEDECLCHECK_H

clang-tidy/readability/IsolateDeclCheck.cpp

+//===--- IsolateDeclCheck.cpp - clang-tidy---------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "IsolateDeclCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+#include <iostream>
+#include <numeric>
+
+using namespace clang::ast_matchers;
+
+#define PRINT_DEBUG 0
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+namespace {
+AST_MATCHER(DeclStmt, isSingleDecl) { return Node.isSingleDecl(); }
+} // namespace
+
+void IsolateDeclCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(declStmt(unless(isSingleDecl())).bind("decl_stmt"), this);
+}
+
+namespace {
+SourceLocation findNextTerminator(SourceLocation Start, const SourceManager &SM,
+                                  const LangOptions &LangOpts) {
+  while (true) {
+    Optional<Token> CurrentToken = Lexer::findNextToken(Start, SM, LangOpts);
+    assert(CurrentToken.hasValue() && "Could not find token in Decl");
+    Token PotentialComma = *CurrentToken;
+
+    if (PotentialComma.getKind() == tok::comma ||
+        PotentialComma.getKind() == tok::semi)
+      return PotentialComma.getLocation();
+
+    Start =
+        Lexer::getLocForEndOfToken(Start, 0, SM, LangOpts).getLocWithOffset(1);
+  }
+}
+
+/// This function goes back in the source text, to find the beginning of
+/// indirection.
+/// Necessary for `int *p, i;` where `p` has one level of indirection.
+/// To isolate the decl properly, the first Slice must only be `int` and not
+/// `int *`.
+SourceLocation findStartOfIndirection(SourceLocation Start,
+                                      const SourceManager &SM,
+                                      const LangOptions &LangOpts) {
+  while (true) {
+    Optional<Token> CurrentToken = Lexer::findNextToken(Start, SM, LangOpts);
+    assert(CurrentToken.hasValue() && "Could not find token in Decl");
+    Token PotentialIndirection = *CurrentToken;
+
+    if (PotentialIndirection.getKind() == tok::star ||
+        PotentialIndirection.getKind() == tok::amp)
+      return PotentialIndirection.getLocation();
+
+    Start =
+        Lexer::getLocForEndOfToken(Start, 0, SM, LangOpts).getLocWithOffset(1);
+  }
+}
+
+/// This function assumes, that \p DS only contains VarDecl elements.
+std::vector<SourceRange> DeclSlice(const DeclStmt *DS, const SourceManager &SM,
+                                   const LangOptions &LangOpts) {
+  // The initial type of the declaration and each declaration has it's slice.
+  std::vector<SourceRange> Slices;
+  Slices.reserve(std::distance(DS->decl_begin(), DS->decl_end()) + 1);
+#if PRINT_DEBUG
+  std::cerr << "Number of Slices to create: " << Slices.capacity() << "\n";
+#endif
+  assert(Slices.capacity() > 2 &&
+         "Expect at least 3 sections (type, first decl, second decl) ");
+
+  // Special case for the type where the SourceRange is computed
+  // differently.
+  VarDecl *FirstDecl = dyn_cast<VarDecl>(*DS->decl_begin());
+  assert(FirstDecl && "Expect only VarDecls");
+
+  SourceLocation Start;
+  if (FirstDecl->getType()->isPointerType() ||
+      FirstDecl->getType()->isReferenceType())
+    Start = findStartOfIndirection(DS->getBeginLoc(), SM, LangOpts);
+  else
+    Start = FirstDecl->getLocation();
+
+#if PRINT_DEBUG
+  std::cerr << "Start of Indirection: " << Start.printToString(SM) << "\n";
+#endif
+  Slices.emplace_back(SourceRange(DS->getBeginLoc(), Start));
+
+  SourceLocation DeclBegin = Start;
+  for (auto It = DS->decl_begin(), End = DS->decl_end(); It != End; ++It) {
+    VarDecl *CurrentDecl = dyn_cast<VarDecl>(*It);
+    assert(CurrentDecl && "Expect only VarDecls here");
+
+    SourceLocation DeclEnd;
+    if (CurrentDecl->hasInit())
+      DeclEnd =
+          findNextTerminator(CurrentDecl->getInit()->getEndLoc(), SM, LangOpts);
+    else
+      DeclEnd = findNextTerminator(CurrentDecl->getEndLoc(), SM, LangOpts);
+
+#if PRINT_DEBUG
+    std::cerr << "Start of Slice: " << DeclBegin.printToString(SM) << "\n";
+    std::cerr << "End of Slice: " << DeclEnd.printToString(SM) << "\n";
+#endif
+    Slices.emplace_back(SourceRange(DeclBegin, DeclEnd));
+    DeclBegin = DeclEnd.getLocWithOffset(1);
+  }
+
+  return Slices;
+}
+
+std::vector<StringRef> SourceFromRanges(const std::vector<SourceRange> &Ranges,
+                                        const SourceManager &SM,
+                                        const LangOptions &LangOpts) {
+  std::vector<StringRef> Snippets;
+  Snippets.reserve(Ranges.size());
+
+  for (const auto &Range : Ranges) {
+    CharSourceRange CR = Lexer::getAsCharRange(
+        CharSourceRange::getCharRange(Range.getBegin(), Range.getEnd()), SM,
+        LangOpts);
+    assert(CR.isValid() && "Retrieved invalid char source range");
+
+    bool InvalidText = false;
+    StringRef Snippet = Lexer::getSourceText(CR, SM, LangOpts, &InvalidText);
+    assert(!InvalidText && "Retrieved invalid text");
+
+    Snippets.emplace_back(Snippet);
+  }
+
+  return Snippets;
+}
+
+/// Excepts a vector {TypeSnippet, Firstdecl, SecondDecl, ...}.
+std::vector<std::string>
+CreateIsolatedDecls(const std::vector<StringRef> &Snippets) {
+  // The first section is the type snippet, which does not make a decl itself.
+  assert(Snippets.size() >= 2 && "No enough snippets to create isolated decls");
+  std::vector<std::string> Decls(Snippets.size() - 1);
+
+  for (std::size_t i = 1, End = Snippets.size(); i < End; ++i)
+    Decls[i - 1] = Snippets[0].str() + Snippets[i].ltrim().str();
+
+  return Decls;
+}
+
+std::string &TrimRight(std::string &str) {
+  auto it1 = std::find_if(str.rbegin(), str.rend(), [](char ch) {
+    return !std::isspace<char>(ch, std::locale::classic());
+  });
+  str.erase(it1.base(), str.end());
+  return str;
+}
+
+std::string Concat(const std::vector<std::string> &Decls, StringRef Indent) {
+  std::string R;
+  for (const std::string &D : Decls)
+    R += D + ";\n" + Indent.str();
+  return TrimRight(R);
+}
+
+} // namespace
+
+void IsolateDeclCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *WholeDecl = Result.Nodes.getNodeAs<DeclStmt>("decl_stmt");
+
+  std::vector<SourceRange> Slices =
+      DeclSlice(WholeDecl, *Result.SourceManager, getLangOpts());
+#if PRINT_DEBUG
+  std::cerr << "Number of Slices: " << Slices.size() << "\n";
+  for (const auto &Slice : Slices) {
+    diag(Slice.getBegin(), "Slice start", DiagnosticIDs::Note);
+    diag(Slice.getEnd(), "Slice End", DiagnosticIDs::Note);
+  }
+#endif
+
+  std::vector<StringRef> Snippets =
+      SourceFromRanges(Slices, *Result.SourceManager, getLangOpts());
+
+  std::vector<std::string> NewDecls = CreateIsolatedDecls(Snippets);
+  std::string Replacement =
+      Concat(NewDecls, Lexer::getIndentationForLine(WholeDecl->getBeginLoc(),
+                                                    *Result.SourceManager));
+
+  std::cerr << Replacement << "\n";
+
+  diag(WholeDecl->getBeginLoc(), "make only one declaration per statement")
+      << FixItHint::CreateReplacement(WholeDecl->getSourceRange(), Replacement);
+}
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "ContainerSizeEmptyCheck.h"
 #include "DeleteNullPointerCheck.h"
 #include "DeletedDefaultCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolConversionCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "IsolateDeclCheck.h"
 #include "MagicNumbersCheck.h"
 #include "MisleadingIndentationCheck.h"
 #include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "NonConstParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantDeclarationCheck.h"
 #include "RedundantFunctionPtrDereferenceCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");
     CheckFactories.registerCheck<IdentifierNamingCheck>(
         "readability-identifier-naming");
     CheckFactories.registerCheck<ImplicitBoolConversionCheck>(
         "readability-implicit-bool-conversion");
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
         "readability-inconsistent-declaration-parameter-name");
+    CheckFactories.registerCheck<IsolateDeclCheck>(
+        "readability-isolate-decl");
     CheckFactories.registerCheck<MagicNumbersCheck>(
         "readability-magic-numbers");
     CheckFactories.registerCheck<MisleadingIndentationCheck>(
         "readability-misleading-indentation");
     CheckFactories.registerCheck<MisplacedArrayIndexCheck>(
         "readability-misplaced-array-index");
     CheckFactories.registerCheck<RedundantFunctionPtrDereferenceCheck>(
         "readability-redundant-function-ptr-dereference");

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 - New :doc:`abseil-duration-division
   <clang-tidy/checks/abseil-duration-division>` check.
 
   Checks for uses of ``absl::Duration`` division that is done in a
   floating-point context, and recommends the use of a function that
   returns a floating-point value.
 
 - New :doc:`abseil-faster-strsplit-delimiter
   <clang-tidy/checks/abseil-faster-strsplit-delimiter>` check.
- New :doc:`abseil-redundant-strcat-calls
   being passed to another ``absl::StrCat`` or ``absl::StrAppend``.
 
 - New :doc:`abseil-str-cat-append
   <clang-tidy/checks/abseil-str-cat-append>` check.
 
   Flags uses of ``absl::StrCat()`` to append to a ``std::string``. Suggests 
   ``absl::StrAppend()`` should be used instead.
 
+- New :doc:`readability-isolate-decl
+  <clang-tidy/checks/readability-isolate-decl>` check.
+
+  FIXME: add release notes.
+
 - New :doc:`readability-magic-numbers
   <clang-tidy/checks/readability-magic-numbers>` check.
 
   Detects usage of magic numbers, numbers that are used as literals instead of
   introduced via constants or symbols.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/list.rst

.. toctree::
    readability-container-size-empty
    readability-delete-null-pointer
    readability-deleted-default
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-conversion
    readability-inconsistent-declaration-parameter-name
+   readability-isolate-decl
    readability-magic-numbers
    readability-misleading-indentation
    readability-misplaced-array-index
    readability-named-parameter
    readability-non-const-parameter
    readability-redundant-control-flow
    readability-redundant-declaration
    readability-redundant-function-ptr-dereference

docs/clang-tidy/checks/readability-isolate-decl.rst

+.. title:: clang-tidy - readability-isolate-decl
+
+readability-isolate-decl
+========================
+
+FIXME: Describe what patterns does the check detect and why. Give examples.

test/clang-tidy/readability-isolate-decl.cpp

+// RUN: %check_clang_tidy %s readability-isolate-decl %t
+
+void f() {
+  int i;
+}
+
+void f2() {
+  int i, j, *k, lala = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: make only one declaration per statement
+  // CHECK-FIXES: int i;
+  // CHECK-FIXES: {{^  }}int j;
+  // CHECK-FIXES: {{^  }}int *k;
+  // CHECK-FIXES: {{^  }}int lala = 42;
+
+  int normal, weird = /* comment */ 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: make only one declaration per statement
+  // CHECK-FIXES: int normal;
+  // CHECK-FIXES: {{^  }}int weird = /* comment */ 42;
+}
+
+void f3() {
+  int i, *pointer1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: make only one declaration per statement
+  // CHECK-FIXES: int i;
+  // CHECK-FIXES: {{^  }}int *pointer1;
+  //
+  int *pointer2 = nullptr, *pointer3 = &i;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: make only one declaration per statement
+  // CHECK-FIXES: int *pointer2 = nullptr;
+  // CHECK-FIXES: {{^  }}int *pointer3 = &i;
+}
+
+void f4() {
+  double d = 42. /* foo */, z = 43., /* hi */ y, c /* */ /*  */, l = 2.;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: make only one declaration per statement
+  // CHECK-FIXES: double d = 42. /* foo */;
+  // CHECK-FIXES: {{^  }}double z = 43.;
+  // CHECK-FIXES: {{^  }}double /* hi */ y;
+  // CHECK-FIXES: {{^  }}double c /* */ /*  */;
+  // CHECK-FIXES: {{^  }}double l = 2.;
+}
+
+struct SomeClass {
+  SomeClass() = default;
+  SomeClass(int value);
+};
+void f5() {
+  SomeClass v1, v2(42), v3{42}, v4(42.5);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: make only one declaration per statement
+  // CHECK-FIXES: SomeClass v1;
+  // CHECK-FIXES: {{^  }}SomeClass v2(42);
+  // CHECK-FIXES: {{^  }}SomeClass v3{42};
+  // CHECK-FIXES: {{^  }}SomeClass v4(42.5);
+
+  SomeClass v5 = 42, *p1 = nullptr;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: make only one declaration per statement
+  // CHECK-FIXES: SomeClass v5 = 42;
+  // CHECK-FIXES: {{^  }}SomeClass *p1 = nullptr;
+}
+
+void f6() {
+  int array1[] = {1, 2, 3, 4}, array2[] = {1, 2, 3}, value1, value2 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: make only one declaration per statement
+  // CHECK-FIXES: int array1[] = {1, 2, 3, 4};
+  // CHECK-FIXES: {{^  }}int array2[] = {1, 2, 3};
+  // CHECK-FIXES: {{^  }}int value1;
+  // CHECK-FIXES: {{^  }}int value2 = 42;
+}
+
+template <typename T>
+struct TemplatedType {
+  TemplatedType() = default;
+  TemplatedType(T value);
+};
+
+void f7() {
+  TemplatedType<int> TT1(42), TT2{42}, TT3;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: make only one declaration per statement
+  // CHECK-FIXES: TemplatedType<int> TT1(42);
+  // CHECK-FIXES: {{^  }}TemplatedType<int> TT2{42};
+  // CHECK-FIXES: {{^  }}TemplatedType<int> TT3;
+}