[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

323 ↗

(On Diff #166044)

llvm::StringRef::rtrim()?

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

331 ↗

(On Diff #166044)

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

333 ↗

(On Diff #166044)

Range?

[JonasToth]

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

clang-tidy/readability/IsolateDeclCheck.cpp

21 ↗

(On Diff #166044)

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

323 ↗

(On Diff #166044)

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

331 ↗

(On Diff #166044)

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

343 ↗

(On Diff #166044)

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

343 ↗

(On Diff #166044)

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

39 ↗

(On Diff #167282)

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

65 ↗

(On Diff #167282)

i-> I

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

68 ↗

(On Diff #167282)

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

78 ↗

(On Diff #167282)

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.

93 ↗

(On Diff #167282)

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

102 ↗

(On Diff #167282)

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

166 ↗

(On Diff #167282)

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.

168 ↗

(On Diff #167282)

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

173 ↗

(On Diff #167282)

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.

208 ↗

(On Diff #167282)

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

211 ↗

(On Diff #167282)

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.

214 ↗

(On Diff #167282)

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

218 ↗

(On Diff #167282)

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

244 ↗

(On Diff #167282)

Same here (llvm::ArrayRef)

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

249 ↗

(On Diff #167282)

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

255 ↗

(On Diff #167282)

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

268 ↗

(On Diff #167282)

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.

273 ↗

(On Diff #167282)

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

Same below.

278 ↗

(On Diff #167282)

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

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

142 ↗

(On Diff #167282)

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

343 ↗

(On Diff #166044)

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

343 ↗

(On Diff #166044)

"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

343 ↗

(On Diff #166044)

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

343 ↗

(On Diff #166044)

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

343 ↗

(On Diff #166044)

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

68 ↗

(On Diff #167282)

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

278 ↗

(On Diff #167282)

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

343 ↗

(On Diff #166044)

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

211 ↗

(On Diff #167282)

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

343 ↗

(On Diff #166044)

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

343 ↗

(On Diff #166044)

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

184 ↗

(On Diff #167437)

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

(On Diff #167491)

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

142 ↗

(On Diff #171361)

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-tools-extra/trunk/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
+  IsolateDeclarationCheck.cpp
   MagicNumbersCheck.cpp
   MisleadingIndentationCheck.cpp
   MisplacedArrayIndexCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   NonConstParameterCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp

clang-tools-extra/trunk/clang-tidy/readability/IsolateDeclarationCheck.h

+//===--- IsolateDeclarationCheck.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 {
+
+/// This check diagnoses all DeclStmt's declaring more than one variable and
+/// tries to refactor the code to one statement per declaration.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-isolate-declaration.html
+class IsolateDeclarationCheck : public ClangTidyCheck {
+public:
+  IsolateDeclarationCheck(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-tools-extra/trunk/clang-tidy/readability/IsolateDeclarationCheck.cpp

+//===--- IsolateDeclarationCheck.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 "IsolateDeclarationCheck.h"
+#include "../utils/LexerUtils.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+using namespace clang::tidy::utils::lexer;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+namespace {
+AST_MATCHER(DeclStmt, isSingleDecl) { return Node.isSingleDecl(); }
+AST_MATCHER(DeclStmt, onlyDeclaresVariables) {
+  return llvm::all_of(Node.decls(), [](Decl *D) { return isa<VarDecl>(D); });
+}
+} // namespace
+
+void IsolateDeclarationCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      declStmt(allOf(onlyDeclaresVariables(), unless(isSingleDecl()),
+                     hasParent(compoundStmt())))
+          .bind("decl_stmt"),
+      this);
+}
+
+static SourceLocation findStartOfIndirection(SourceLocation Start,
+                                             int Indirections,
+                                             const SourceManager &SM,
+                                             const LangOptions &LangOpts) {
+  assert(Indirections >= 0 && "Indirections must be non-negative");
+  if (Indirections == 0)
+    return Start;
+
+  // Note that the post-fix decrement is necessary to perform the correct
+  // number of transformations.
+  while (Indirections-- != 0) {
+    Start = findPreviousAnyTokenKind(Start, SM, LangOpts, tok::star, tok::amp);
+    if (Start.isInvalid() || Start.isMacroID())
+      return SourceLocation();
+  }
+  return Start;
+}
+
+static bool isMacroID(SourceRange R) {
+  return R.getBegin().isMacroID() || R.getEnd().isMacroID();
+}
+
+/// This function counts the number of written indirections for the given
+/// Type \p T. It does \b NOT resolve typedefs as it's a helper for lexing
+/// the source code.
+/// \see declRanges
+static int countIndirections(const Type *T, int Indirections = 0) {
+  if (T->isFunctionPointerType()) {
+    const auto *Pointee = T->getPointeeType()->castAs<FunctionType>();
+    return countIndirections(
+        Pointee->getReturnType().IgnoreParens().getTypePtr(), ++Indirections);
+  }
+
+  // Note: Do not increment the 'Indirections' because it is not yet clear
+  // if there is an indirection added in the source code of the array
+  // declaration.
+  if (const auto *AT = dyn_cast<ArrayType>(T))
+    return countIndirections(AT->getElementType().IgnoreParens().getTypePtr(),
+                             Indirections);
+
+  if (isa<PointerType>(T) || isa<ReferenceType>(T))
+    return countIndirections(T->getPointeeType().IgnoreParens().getTypePtr(),
+                             ++Indirections);
+
+  return Indirections;
+}
+
+static bool typeIsMemberPointer(const Type *T) {
+  if (isa<ArrayType>(T))
+    return typeIsMemberPointer(T->getArrayElementTypeNoTypeQual());
+
+  if ((isa<PointerType>(T) || isa<ReferenceType>(T)) &&
+      isa<PointerType>(T->getPointeeType()))
+    return typeIsMemberPointer(T->getPointeeType().getTypePtr());
+
+  return isa<MemberPointerType>(T);
+}
+
+/// This function tries to extract the SourceRanges that make up all
+/// declarations in this \c DeclStmt.
+///
+/// The resulting vector has the structure {UnderlyingType, Decl1, Decl2, ...}.
+/// Each \c SourceRange is of the form [Begin, End).
+/// If any of the create ranges is invalid or in a macro the result will be
+/// \c None.
+/// If the \c DeclStmt contains only one declaration, the result is \c None.
+/// If the \c DeclStmt contains declarations other than \c VarDecl the result
+/// is \c None.
+///
+/// \code
+///    int * ptr1 = nullptr, value = 42;
+/// // [  ][              ] [         ] - The ranges here are inclusive
+/// \endcode
+/// \todo Generalize this function to take other declarations than \c VarDecl.
+static Optional<std::vector<SourceRange>>
+declRanges(const DeclStmt *DS, const SourceManager &SM,
+           const LangOptions &LangOpts) {
+  std::size_t DeclCount = std::distance(DS->decl_begin(), DS->decl_end());
+  if (DeclCount < 2)
+    return None;
+
+  if (rangeContainsExpansionsOrDirectives(DS->getSourceRange(), SM, LangOpts))
+    return None;
+
+  // The initial type of the declaration and each declaration has it's own
+  // slice. This is necessary, because pointers and references bind only
+  // to the local variable and not to all variables in the declaration.
+  // Example: 'int *pointer, value = 42;'
+  std::vector<SourceRange> Slices;
+  Slices.reserve(DeclCount + 1);
+
+  // Calculate the first slice, for now only variables are handled but in the
+  // future this should be relaxed and support various kinds of declarations.
+  const auto *FirstDecl = dyn_cast<VarDecl>(*DS->decl_begin());
+
+  if (FirstDecl == nullptr)
+    return None;
+
+  // FIXME: Member pointers are not transformed correctly right now, that's
+  // why they are treated as problematic here.
+  if (typeIsMemberPointer(FirstDecl->getType().IgnoreParens().getTypePtr()))
+    return None;
+
+  // Consider the following case: 'int * pointer, value = 42;'
+  // Created slices (inclusive)    [  ][       ] [         ]
+  // Because 'getBeginLoc' points to the start of the variable *name*, the
+  // location of the pointer must be determined separatly.
+  SourceLocation Start = findStartOfIndirection(
+      FirstDecl->getLocation(),
+      countIndirections(FirstDecl->getType().IgnoreParens().getTypePtr()), SM,
+      LangOpts);
+
+  // Fix function-pointer declarations that have a '(' in front of the
+  // pointer.
+  // Example: 'void (*f2)(int), (*g2)(int, float) = gg;'
+  // Slices:   [   ][        ] [                     ]
+  if (FirstDecl->getType()->isFunctionPointerType())
+    Start = findPreviousTokenKind(Start, SM, LangOpts, tok::l_paren);
+
+  // It is popssible that a declarator is wrapped with parens.
+  // Example: 'float (((*f_ptr2)))[42], *f_ptr3, ((f_value2)) = 42.f;'
+  // The slice for the type-part must not contain these parens. Consequently
+  // 'Start' is moved to the most left paren if there are parens.
+  while (true) {
+    if (Start.isInvalid() || Start.isMacroID())
+      break;
+
+    Token T = getPreviousToken(Start, SM, LangOpts);
+    if (T.is(tok::l_paren)) {
+      Start = findPreviousTokenStart(Start, SM, LangOpts);
+      continue;
+    }
+    break;
+  }
+
+  SourceRange DeclRange(DS->getBeginLoc(), Start);
+  if (DeclRange.isInvalid() || isMacroID(DeclRange))
+    return None;
+
+  // The first slice, that is prepended to every isolated declaration, is
+  // created.
+  Slices.emplace_back(DeclRange);
+
+  // Create all following slices that each declare a variable.
+  SourceLocation DeclBegin = Start;
+  for (const auto &Decl : DS->decls()) {
+    const auto *CurrentDecl = cast<VarDecl>(Decl);
+
+    // FIXME: Member pointers are not transformed correctly right now, that's
+    // why they are treated as problematic here.
+    if (typeIsMemberPointer(CurrentDecl->getType().IgnoreParens().getTypePtr()))
+      return None;
+
+    SourceLocation DeclEnd =
+        CurrentDecl->hasInit()
+            ? findNextTerminator(CurrentDecl->getInit()->getEndLoc(), SM,
+                                 LangOpts)
+            : findNextTerminator(CurrentDecl->getEndLoc(), SM, LangOpts);
+
+    SourceRange VarNameRange(DeclBegin, DeclEnd);
+    if (VarNameRange.isInvalid() || isMacroID(VarNameRange))
+      return None;
+
+    Slices.emplace_back(VarNameRange);
+    DeclBegin = DeclEnd.getLocWithOffset(1);
+  }
+  return Slices;
+}
+
+static Optional<std::vector<StringRef>>
+collectSourceRanges(llvm::ArrayRef<SourceRange> Ranges, const SourceManager &SM,
+                    const LangOptions &LangOpts) {
+  std::vector<StringRef> Snippets;
+  Snippets.reserve(Ranges.size());
+
+  for (const auto &Range : Ranges) {
+    CharSourceRange CharRange = Lexer::getAsCharRange(
+        CharSourceRange::getCharRange(Range.getBegin(), Range.getEnd()), SM,
+        LangOpts);
+
+    if (CharRange.isInvalid())
+      return None;
+
+    bool InvalidText = false;
+    StringRef Snippet =
+        Lexer::getSourceText(CharRange, SM, LangOpts, &InvalidText);
+
+    if (InvalidText)
+      return None;
+
+    Snippets.emplace_back(Snippet);
+  }
+
+  return Snippets;
+}
+
+/// Expects a vector {TypeSnippet, Firstdecl, SecondDecl, ...}.
+static std::vector<std::string>
+createIsolatedDecls(llvm::ArrayRef<StringRef> Snippets) {
+  // The first section is the type snippet, which does not make a decl itself.
+  assert(Snippets.size() > 2 && "Not enough snippets to create isolated decls");
+  std::vector<std::string> Decls(Snippets.size() - 1);
+
+  for (std::size_t I = 1; I < Snippets.size(); ++I)
+    Decls[I - 1] = Twine(Snippets[0])
+                       .concat(Snippets[0].endswith(" ") ? "" : " ")
+                       .concat(Snippets[I].ltrim())
+                       .concat(";")
+                       .str();
+
+  return Decls;
+}
+
+void IsolateDeclarationCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *WholeDecl = Result.Nodes.getNodeAs<DeclStmt>("decl_stmt");
+
+  auto Diag =
+      diag(WholeDecl->getBeginLoc(),
+           "multiple declarations in a single statement reduces readability");
+
+  Optional<std::vector<SourceRange>> PotentialRanges =
+      declRanges(WholeDecl, *Result.SourceManager, getLangOpts());
+  if (!PotentialRanges)
+    return;
+
+  Optional<std::vector<StringRef>> PotentialSnippets = collectSourceRanges(
+      *PotentialRanges, *Result.SourceManager, getLangOpts());
+
+  if (!PotentialSnippets)
+    return;
+
+  std::vector<std::string> NewDecls = createIsolatedDecls(*PotentialSnippets);
+  std::string Replacement = llvm::join(
+      NewDecls,
+      (Twine("\n") + Lexer::getIndentationForLine(WholeDecl->getBeginLoc(),
+                                                  *Result.SourceManager))
+          .str());
+
+  Diag << FixItHint::CreateReplacement(WholeDecl->getSourceRange(),
+                                       Replacement);
+}
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/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 "IsolateDeclarationCheck.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<IsolateDeclarationCheck>(
+        "readability-isolate-declaration");
     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");

clang-tools-extra/trunk/clang-tidy/utils/LexerUtils.h

 namespace tidy {
 namespace utils {
 namespace lexer {
 
 /// Returns previous token or ``tok::unknown`` if not found.
 Token getPreviousToken(SourceLocation Location, const SourceManager &SM,
                        const LangOptions &LangOpts, bool SkipComments = true);
 
+SourceLocation findPreviousTokenStart(SourceLocation Start,
+                                      const SourceManager &SM,
+                                      const LangOptions &LangOpts);
+
+SourceLocation findPreviousTokenKind(SourceLocation Start,
+                                     const SourceManager &SM,
+                                     const LangOptions &LangOpts,
+                                     tok::TokenKind TK);
+
+SourceLocation findNextTerminator(SourceLocation Start, const SourceManager &SM,
+                                  const LangOptions &LangOpts);
+
+template <typename TokenKind, typename... TokenKinds>
+SourceLocation findPreviousAnyTokenKind(SourceLocation Start,
+                                        const SourceManager &SM,
+                                        const LangOptions &LangOpts,
+                                        TokenKind TK, TokenKinds... TKs) {
+  while (true) {
+    SourceLocation L = findPreviousTokenStart(Start, SM, LangOpts);
+    if (L.isInvalid() || L.isMacroID())
+      return SourceLocation();
+
+    Token T;
+    // Returning 'true' is used to signal failure to retrieve the token.
+    if (Lexer::getRawToken(L, T, SM, LangOpts))
+      return SourceLocation();
+
+    if (T.isOneOf(TK, TKs...))
+      return T.getLocation();
+
+    Start = L;
+  }
+}
+
+template <typename TokenKind, typename... TokenKinds>
+SourceLocation findNextAnyTokenKind(SourceLocation Start,
+                                    const SourceManager &SM,
+                                    const LangOptions &LangOpts, TokenKind TK,
+                                    TokenKinds... TKs) {
+  while (true) {
+    Optional<Token> CurrentToken = Lexer::findNextToken(Start, SM, LangOpts);
+
+    if (!CurrentToken)
+      return SourceLocation();
+
+    Token PotentialMatch = *CurrentToken;
+    if (PotentialMatch.isOneOf(TK, TKs...))
+      return PotentialMatch.getLocation();
+
+    Start = PotentialMatch.getLastLoc();
+  }
+}
+
+/// Re-lex the provide \p Range and return \c false if either a macro spans
+/// multiple tokens, a pre-processor directive or failure to retrieve the
+/// next token is found, otherwise \c true.
+bool rangeContainsExpansionsOrDirectives(SourceRange Range,
+                                         const SourceManager &SM,
+                                         const LangOptions &LangOpts);
+
 } // namespace lexer
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_LEXER_UTILS_H

clang-tools-extra/trunk/clang-tidy/utils/LexerUtils.cpp

if (!Lexer::getRawToken(Location, Token, SM, LangOpts) &&
         (!SkipComments || !Token.is(tok::comment))) {
       break;
     }
     Location = Location.getLocWithOffset(-1);
   }
   return Token;
 }
 
+SourceLocation findPreviousTokenStart(SourceLocation Start,
+                                      const SourceManager &SM,
+                                      const LangOptions &LangOpts) {
+  if (Start.isInvalid() || Start.isMacroID())
+    return SourceLocation();
+
+  SourceLocation BeforeStart = Start.getLocWithOffset(-1);
+  if (BeforeStart.isInvalid() || BeforeStart.isMacroID())
+    return SourceLocation();
+
+  return Lexer::GetBeginningOfToken(BeforeStart, SM, LangOpts);
+}
+
+SourceLocation findPreviousTokenKind(SourceLocation Start,
+                                     const SourceManager &SM,
+                                     const LangOptions &LangOpts,
+                                     tok::TokenKind TK) {
+  while (true) {
+    SourceLocation L = findPreviousTokenStart(Start, SM, LangOpts);
+    if (L.isInvalid() || L.isMacroID())
+      return SourceLocation();
+
+    Token T;
+    if (Lexer::getRawToken(L, T, SM, LangOpts, /*IgnoreWhiteSpace=*/true))
+      return SourceLocation();
+
+    if (T.is(TK))
+      return T.getLocation();
+
+    Start = L;
+  }
+}
+
+SourceLocation findNextTerminator(SourceLocation Start, const SourceManager &SM,
+                                  const LangOptions &LangOpts) {
+  return findNextAnyTokenKind(Start, SM, LangOpts, tok::comma, tok::semi);
+}
+
+bool rangeContainsExpansionsOrDirectives(SourceRange Range,
+                                         const SourceManager &SM,
+                                         const LangOptions &LangOpts) {
+  assert(Range.isValid() && "Invalid Range for relexing provided");
+  SourceLocation Loc = Range.getBegin();
+
+  while (Loc < Range.getEnd()) {
+    if (Loc.isMacroID())
+      return true;
+
+    llvm::Optional<Token> Tok = Lexer::findNextToken(Loc, SM, LangOpts);
+
+    if (!Tok)
+      return true;
+
+    if (Tok->is(tok::hash))
+      return true;
+
+    Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, LangOpts).getLocWithOffset(1);
+  }
+
+  return false;
+}
 } // namespace lexer
 } // namespace utils
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New :doc:`modernize-concat-nested-namespaces
   syntax introduced in C++17 standard: ``namespace a::b { ... }``.
 
 - New :doc:`modernize-deprecated-ios-base-aliases
   <clang-tidy/checks/modernize-deprecated-ios-base-aliases>` check.
 
   Detects usage of the deprecated member types of ``std::ios_base`` and replaces
   those that have a non-deprecated equivalent.
 
+- New :doc:`readability-isolate-decl
+  <clang-tidy/checks/readability-isolate-decl>` check.
+
+  Detects local variable declarations declaring more than one variable and
+  tries to refactor the code to one statement per declaration.
+
 - 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.
 
 - New :doc:`readability-uppercase-literal-suffix
   <clang-tidy/checks/readability-uppercase-literal-suffix>` check.

clang-tools-extra/trunk/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-declaration
    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

clang-tools-extra/trunk/docs/clang-tidy/checks/readability-isolate-declaration.rst

+.. title:: clang-tidy - readability-isolate-declaration
+
+readability-isolate-declaration
+===============================
+
+Detects local variable declarations declaring more than one variable and
+tries to refactor the code to one statement per declaration.
+
+The automatic code-transformation will use the same indentation as the original
+for every created statement and add a line break after each statement.
+It keeps the order of the variable declarations consistent, too.
+
+.. code-block:: c++
+
+  void f() {
+    int * pointer = nullptr, value = 42, * const const_ptr = &value;
+    // This declaration will be diagnosed and transformed into:
+    // int * pointer = nullptr;
+    // int value = 42;
+    // int * const const_ptr = &value;
+  }
+
+
+The check excludes places where it is necessary or common to declare
+multiple variables in one statement and there is no other way supported in the
+language. Please note that structured bindings are not considered.
+
+.. code-block:: c++
+
+  // It is not possible to transform this declaration and doing the declaration
+  // before the loop will increase the scope of the variable 'Begin' and 'End'
+  // which is undesirable.
+  for (int Begin = 0, End = 100; Begin < End; ++Begin);
+  if (int Begin = 42, Result = some_function(Begin); Begin == Result);
+
+  // It is not possible to transform this declaration because the result is
+  // not functionality preserving as 'j' and 'k' would not be part of the
+  // 'if' statement anymore.
+  if (SomeCondition())
+    int i = 42, j = 43, k = function(i,j);
+
+
+Limitations
+-----------
+
+Global variables and member variables are excluded.
+
+The check currently does not support the automatic transformation of
+member-pointer-types.
+
+.. code-block:: c++
+
+  struct S {
+    int a;
+    const int b;
+    void f() {}
+  };
+
+  void f() {
+    // Only a diagnostic message is emitted
+    int S::*p = &S::a, S::*const q = &S::a;
+  }
+
+Furthermore, the transformation is very cautious when it detects various kinds
+of macros or preprocessor directives in the range of the statement. In this
+case the transformation will not happen to avoid unexpected side-effects due to
+macros.
+
+.. code-block:: c++
+
+  #define NULL 0
+  #define MY_NICE_TYPE int **
+  #define VAR_NAME(name) name##__LINE__
+  #define A_BUNCH_OF_VARIABLES int m1 = 42, m2 = 43, m3 = 44;
+
+  void macros() {
+    int *p1 = NULL, *p2 = NULL;
+    // Will be transformed to
+    // int *p1 = NULL;
+    // int *p2 = NULL;
+
+    MY_NICE_TYPE p3, v1, v2;
+    // Won't be transformed, but a diagnostic is emitted.
+
+    int VAR_NAME(v3),
+        VAR_NAME(v4),
+        VAR_NAME(v5);
+    // Won't be transformed, but a diagnostic is emitted.
+
+    A_BUNCH_OF_VARIABLES
+    // Won't be transformed, but a diagnostic is emitted.
+
+    int Unconditional,
+  #if CONFIGURATION
+        IfConfigured = 42,
+  #else
+        IfConfigured = 0;
+  #endif
+    // Won't be transformed, but a diagnostic is emitted.
+  }

clang-tools-extra/trunk/test/clang-tidy/readability-isolate-declaration-cxx17.cpp

+// RUN: %check_clang_tidy %s readability-isolate-declaration %t -- -- -std=c++17
+
+template <typename T1, typename T2>
+struct pair {
+  T1 first;
+  T2 second;
+  pair(T1 v1, T2 v2) : first(v1), second(v2) {}
+
+  template <int N>
+  decltype(auto) get() const {
+    if constexpr (N == 0)
+      return first;
+    else if constexpr (N == 1)
+      return second;
+  }
+};
+
+void forbidden_transformations() {
+  if (int i = 42, j = i; i == j)
+    ;
+  switch (int i = 12, j = 14; i)
+    ;
+
+  auto [i, j] = pair<int, int>(42, 42);
+}
+
+struct SomeClass {
+  SomeClass() = default;
+  SomeClass(int value);
+};
+
+namespace std {
+template <typename T>
+class initializer_list {};
+
+template <typename T>
+class vector {
+public:
+  vector() = default;
+  vector(initializer_list<T> init) {}
+};
+
+class string {
+public:
+  string() = default;
+  string(const char *) {}
+};
+
+namespace string_literals {
+string operator""s(const char *, decltype(sizeof(int))) {
+  return string();
+}
+} // namespace string_literals
+} // namespace std
+
+namespace Types {
+typedef int MyType;
+} // namespace Types
+
+int touch1, touch2;
+
+void modern() {
+  auto autoInt1 = 3, autoInt2 = 4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: auto autoInt1 = 3;
+  // CHECK-FIXES: {{^  }}auto autoInt2 = 4;
+
+  decltype(int()) declnottouch = 4;
+  decltype(int()) declint1 = 5, declint2 = 3;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: decltype(int()) declint1 = 5;
+  // CHECK-FIXES: {{^  }}decltype(int()) declint2 = 3;
+
+  std::vector<int> vectorA = {1, 2}, vectorB = {1, 2, 3}, vectorC({1, 1, 1});
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: std::vector<int> vectorA = {1, 2};
+  // CHECK-FIXES: {{^  }}std::vector<int> vectorB = {1, 2, 3};
+  // CHECK-FIXES: {{^  }}std::vector<int> vectorC({1, 1, 1});
+
+  using uType = int;
+  uType utype1, utype2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: uType utype1;
+  // CHECK-FIXES: {{^  }}uType utype2;
+
+  Types::MyType mytype1, mytype2, mytype3 = 3;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: Types::MyType mytype1;
+  // CHECK-FIXES: {{^  }}Types::MyType mytype2;
+  // CHECK-FIXES: {{^  }}Types::MyType mytype3 = 3;
+
+  {
+    using namespace std::string_literals;
+
+    std::vector<std::string> s{"foo"s, "bar"s}, t{"foo"s}, u, a({"hey", "you"}), bb = {"h", "a"};
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: multiple declarations in a single statement reduces readability
+    // CHECK-FIXES: std::vector<std::string> s{"foo"s, "bar"s};
+    // CHECK-FIXES: {{^    }}std::vector<std::string> t{"foo"s};
+    // CHECK-FIXES: {{^    }}std::vector<std::string> u;
+    // CHECK-FIXES: {{^    }}std::vector<std::string> a({"hey", "you"});
+    // CHECK-FIXES: {{^    }}std::vector<std::string> bb = {"h", "a"};
+  }
+}

clang-tools-extra/trunk/test/clang-tidy/readability-isolate-declaration-fixing.cpp

+// RUN: %check_clang_tidy %s readability-isolate-declaration %t
+// XFAIL: *
+
+struct S {
+  int a;
+  const int b;
+  void f() {}
+};
+
+void member_pointers() {
+  // FIXME: Memberpointers are transformed incorrect. Emit only a warning
+  // for now.
+  int S::*p = &S::a, S::*const q = &S::a;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: int S::*p = &S::a;
+  // CHECK-FIXES: {{^  }}int S::*const q = &S::a;
+
+  int /* :: */ S::*pp2 = &S::a, var1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: int /* :: */ S::*pp2 = &S::a;
+  // CHECK-FIXES: {{^  }}int var1 = 0;
+
+  const int S::*r = &S::b, S::*t;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: const int S::*r = &S::b;
+  // CHECK-FIXES: {{^  }}const int S::*t;
+
+  {
+    int S::*mdpa1[2] = {&S::a, &S::a}, var1 = 0;
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: multiple declarations in a single statement reduces readability
+    // CHECK-FIXES: int S::*mdpa1[2] = {&S::a, &S::a};
+    // CHECK-FIXES: {{^    }}int var1 = 0;
+
+    int S ::**mdpa2[2] = {&p, &pp2}, var2 = 0;
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: multiple declarations in a single statement reduces readability
+    // CHECK-FIXES: int S ::**mdpa2[2] = {&p, &pp2};
+    // CHECK-FIXES: {{^    }}int var2 = 0;
+
+    void (S::*mdfp1)() = &S::f, (S::*mdfp2)() = &S::f;
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: multiple declarations in a single statement reduces readability
+    // CHECK-FIXES: void (S::*mdfp1)() = &S::f;
+    // CHECK-FIXES: {{^    }}void (S::*mdfp2)() = &S::f;
+
+    void (S::*mdfpa1[2])() = {&S::f, &S::f}, (S::*mdfpa2)() = &S::f;
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: multiple declarations in a single statement reduces readability
+    // CHECK-FIXES: void (S::*mdfpa1[2])() = {&S::f, &S::f};
+    // CHECK-FIXES: {{^    }}void (S::*mdfpa2)() = &S::f;
+
+    void (S::* * mdfpa3[2])() = {&mdfpa1[0], &mdfpa1[1]}, (S::*mdfpa4)() = &S::f;
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: multiple declarations in a single statement reduces readability
+    // CHECK-FIXES: void (S::* * mdfpa3[2])() = {&mdfpa1[0], &mdfpa1[1]};
+    // CHECK-FIXES: {{^    }}void (S::*mdfpa4)() = &S::f;
+  }
+
+  class CS {
+  public:
+    int a;
+    const int b;
+  };
+  int const CS ::*pp = &CS::a, CS::*const qq = &CS::a;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: int const CS ::*pp = &CS::a;
+  // CHECK-FIXES: {{^  }}int const CS::*const qq = &CS::a;
+}

clang-tools-extra/trunk/test/clang-tidy/readability-isolate-declaration.c

+// RUN: %check_clang_tidy %s readability-isolate-declaration %t
+
+void c_specific() {
+  void (*signal(int sig, void (*func)(int)))(int);
+  int i = sizeof(struct S { int i; });
+  int j = sizeof(struct T { int i; }), k;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: int j = sizeof(struct T { int i; });
+  // CHECK-FIXES: {{^  }}int k;
+
+  void g(struct U { int i; } s);                // One decl
+  void h(struct V { int i; } s), m(int i, ...); // Two decls
+}

clang-tools-extra/trunk/test/clang-tidy/readability-isolate-declaration.cpp

+// RUN: %check_clang_tidy %s readability-isolate-declaration %t
+
+void f() {
+  int i;
+}
+
+void f2() {
+  int i, j, *k, lala = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // 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: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: int normal;
+  // CHECK-FIXES: {{^  }}int weird = /* comment */ 42;
+
+  int /* here is a comment */ v1,
+      // another comment
+      v2 = 42 // Ok, more comments
+      ;
+  // CHECK-MESSAGES: [[@LINE-4]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: int /* here is a comment */ v1;
+  // CHECK-FIXES: {{^  }}int /* here is a comment */ // another comment
+  // CHECK-FIXES: {{^      }}v2 = 42 // Ok, more comments
+  // CHECK-FIXES: {{^      }};
+
+  auto int1 = 42, int2 = 0, int3 = 43;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: auto int1 = 42;
+  // CHECK-FIXES: {{^  }}auto int2 = 0;
+  // CHECK-FIXES: {{^  }}auto int3 = 43;
+
+  decltype(auto) ptr1 = &int1, ptr2 = &int1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: decltype(auto) ptr1 = &int1;
+  // CHECK-FIXES: {{^  }}decltype(auto) ptr2 = &int1;
+
+  decltype(k) ptr3 = &int1, ptr4 = &int1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: decltype(k) ptr3 = &int1;
+  // CHECK-FIXES: {{^  }}decltype(k) ptr4 = &int1;
+}
+
+void f3() {
+  int i, *pointer1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: int i;
+  // CHECK-FIXES: {{^  }}int *pointer1;
+  //
+  int *pointer2 = nullptr, *pointer3 = &i;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: int *pointer2 = nullptr;
+  // CHECK-FIXES: {{^  }}int *pointer3 = &i;
+
+  int *(i_ptr) = nullptr, *((i_ptr2));
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: int *(i_ptr) = nullptr;
+  // CHECK-FIXES: {{^  }}int *((i_ptr2));
+
+  float(*f_ptr)[42], (((f_value))) = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: float (*f_ptr)[42];
+  // CHECK-FIXES: {{^  }}float (((f_value))) = 42;
+
+  float(((*f_ptr2)))[42], ((*f_ptr3)), f_value2 = 42.f;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: float (((*f_ptr2)))[42];
+  // CHECK-FIXES: {{^  }}float ((*f_ptr3));
+  // CHECK-FIXES: {{^  }}float f_value2 = 42.f;
+
+  float(((*f_ptr4)))[42], *f_ptr5, ((f_value3));
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: float (((*f_ptr4)))[42];
+  // CHECK-FIXES: {{^  }}float *f_ptr5;
+  // CHECK-FIXES: {{^  }}float ((f_value3));
+
+  void(((*f2))(int)), (*g2)(int, float);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: void (((*f2))(int));
+  // CHECK-FIXES: {{^  }}void (*g2)(int, float);
+
+  float(*(*(*f_ptr6)))[42], (*f_ptr7);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: float (*(*(*f_ptr6)))[42];
+  // CHECK-FIXES: {{^  }}float (*f_ptr7);
+}
+
+void f4() {
+  double d = 42. /* foo */, z = 43., /* hi */ y, c /* */ /*  */, l = 2.;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // 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);
+};
+
+class Point {
+  double x;
+  double y;
+
+public:
+  Point(double x, double y) : x(x), y(y) {}
+};
+
+class Rectangle {
+  Point TopLeft;
+  Point BottomRight;
+
+public:
+  Rectangle(Point TopLeft, Point BottomRight) : TopLeft(TopLeft), BottomRight(BottomRight) {}
+};
+
+void f5() {
+  SomeClass v1, v2(42), v3{42}, v4(42.5);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // 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: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: SomeClass v5 = 42;
+  // CHECK-FIXES: {{^  }}SomeClass *p1 = nullptr;
+
+  Point P1(0., 2.), P2{2., 0.};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: Point P1(0., 2.);
+  // CHECK-FIXES: {{^  }}Point P2{2., 0.};
+
+  Rectangle R1({0., 0.}, {1., -2.}), R2{{0., 1.}, {1., 0.}}, R3(P1, P2), R4{P1, P2};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: Rectangle R1({0., 0.}, {1., -2.});
+  // CHECK-FIXES: {{^  }}Rectangle R2{{[{][{]}}0., 1.}, {1., 0.{{[}][}]}};
+  // CHECK-FIXES: {{^  }}Rectangle R3(P1, P2);
+  // CHECK-FIXES: {{^  }}Rectangle R4{P1, P2};
+}
+
+void f6() {
+  int array1[] = {1, 2, 3, 4}, array2[] = {1, 2, 3}, value1, value2 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // 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: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: TemplatedType<int> TT1(42);
+  // CHECK-FIXES: {{^  }}TemplatedType<int> TT2{42};
+  // CHECK-FIXES: {{^  }}TemplatedType<int> TT3;
+  //
+  TemplatedType<int *> *TT4(nullptr), TT5, **TT6 = nullptr, *const *const TT7{nullptr};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: TemplatedType<int *> *TT4(nullptr);
+  // CHECK-FIXES: {{^  }}TemplatedType<int *> TT5;
+  // CHECK-FIXES: {{^  }}TemplatedType<int *> **TT6 = nullptr;
+  // CHECK-FIXES: {{^  }}TemplatedType<int *> *const *const TT7{nullptr};
+
+  TemplatedType<int &> **TT8(nullptr), *TT9;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: TemplatedType<int &> **TT8(nullptr);
+  // CHECK-FIXES: {{^  }}TemplatedType<int &> *TT9;
+
+  TemplatedType<int *> TT10{nullptr}, *TT11(nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: TemplatedType<int *> TT10{nullptr};
+  // CHECK-FIXES: {{^  }}TemplatedType<int *> *TT11(nullptr);
+}
+
+void forbidden_transformations() {
+  for (int i = 0, j = 42; i < j; ++i)
+    ;
+}
+
+#define NULL 0
+#define MY_NICE_TYPE int **
+#define VAR_NAME(name) name##__LINE__
+#define A_BUNCH_OF_VARIABLES int m1 = 42, m2 = 43, m3 = 44;
+
+void macros() {
+  int *p1 = NULL, *p2 = NULL;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: int *p1 = NULL;
+  // CHECK-FIXES: {{^  }}int *p2 = NULL;
+
+  // Macros are involved, so there will be no transformation
+  MY_NICE_TYPE p3, v1, v2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+
+  int VAR_NAME(v3),
+      VAR_NAME(v4),
+      VAR_NAME(v5);
+  // CHECK-MESSAGES: [[@LINE-3]]:3: warning: multiple declarations in a single statement reduces readability
+
+  A_BUNCH_OF_VARIABLES
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+
+  int Unconditional,
+  // Explanatory comment.
+#if CONFIGURATION
+      IfConfigured = 42,
+#else
+      IfConfigured = 0;
+#endif
+  // CHECK-MESSAGES: [[@LINE-7]]:3: warning: multiple declarations in a single statement reduces readability
+}
+
+void dontTouchParameter(int param1, int param2) {}
+
+struct StructOne {
+  StructOne() {}
+  StructOne(int b) {}
+
+  int member1, member2;
+  // TODO: Handle FieldDecl's as well
+};
+
+using PointerType = int;
+
+struct {
+  int i;
+} AS1, AS2;
+struct TemT {
+  template <typename T>
+  T *getAs() {
+    return nullptr;
+  }
+} TT1, TT2;
+
+void complex_typedefs() {
+  typedef int *IntPtr;
+  typedef int ArrayType[2];
+  typedef int FunType(void);
+
+  IntPtr intptr1, intptr2 = nullptr, intptr3;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: IntPtr intptr1;
+  // CHECK-FIXES: {{^  }}IntPtr intptr2 = nullptr;
+  // CHECK-FIXES: {{^  }}IntPtr intptr3;
+
+  IntPtr *DoublePtr1 = nullptr, **TriplePtr, SinglePtr = nullptr;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: IntPtr *DoublePtr1 = nullptr;
+  // CHECK-FIXES: {{^  }}IntPtr **TriplePtr;
+  // CHECK-FIXES: {{^  }}IntPtr SinglePtr = nullptr;
+
+  IntPtr intptr_array1[2], intptr_array2[4] = {nullptr, nullptr, nullptr, nullptr};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: IntPtr intptr_array1[2];
+  // CHECK-FIXES: {{^  }}IntPtr intptr_array2[4] = {nullptr, nullptr, nullptr, nullptr};
+
+  ArrayType arraytype1, arraytype2 = {1}, arraytype3;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: ArrayType arraytype1;
+  // CHECK-FIXES: {{^  }}ArrayType arraytype2 = {1};
+  // CHECK-FIXES: {{^  }}ArrayType arraytype3;
+
+  // Don't touch function declarations.
+  FunType funtype1, funtype2, functype3;
+
+  for (int index1 = 0, index2 = 0;;) {
+    int localFor1 = 1, localFor2 = 2;
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: multiple declarations in a single statement reduces readability
+    // CHECK-FIXES: int localFor1 = 1;
+    // CHECK-FIXES: {{^    }}int localFor2 = 2;
+  }
+
+  StructOne s1, s2(23), s3, s4(3), *sptr = new StructOne(2);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: StructOne s1;
+  // CHECK-FIXES: {{^  }}StructOne s2(23);
+  // CHECK-FIXES: {{^  }}StructOne s3;
+  // CHECK-FIXES: {{^  }}StructOne s4(3);
+  // CHECK-FIXES: {{^  }}StructOne *sptr = new StructOne(2);
+
+  struct StructOne cs1, cs2(42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: struct StructOne cs1;
+  // CHECK-FIXES: {{^  }}struct StructOne cs2(42);
+
+  int *ptrArray[3], dummy, **ptrArray2[5], twoDim[2][3], *twoDimPtr[2][3];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: int *ptrArray[3];
+  // CHECK-FIXES: {{^  }}int dummy;
+  // CHECK-FIXES: {{^  }}int **ptrArray2[5];
+  // CHECK-FIXES: {{^  }}int twoDim[2][3];
+  // CHECK-FIXES: {{^  }}int *twoDimPtr[2][3];
+
+  {
+    void f1(int), g1(int, float);
+  }
+
+  {
+    void gg(int, float);
+
+    void (*f2)(int), (*g2)(int, float) = gg;
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: multiple declarations in a single statement reduces readability
+    // CHECK-FIXES: void (*f2)(int);
+    // CHECK-FIXES: {{^    }}void (*g2)(int, float) = gg;
+
+    void /*(*/ (/*(*/ *f3)(int), (*g3)(int, float);
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: multiple declarations in a single statement reduces readability
+    // CHECK-FIXES: void /*(*/ (/*(*/ *f3)(int);
+    // CHECK-FIXES: {{^    }}void /*(*/ (*g3)(int, float);
+  }
+
+  // clang-format off
+  auto returner = []() { return int(32); };
+  int intfunction = returner(), intarray[] =
+          {
+                  1,
+                  2,
+                  3,
+                  4
+          }, bb = 4;
+  // CHECK-MESSAGES: [[@LINE-7]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: int intfunction = returner();
+  // CHECK-FIXES: {{^  }}int intarray[] =
+  // CHECK-FIXES: {{^          }}{
+  // CHECK-FIXES: {{^                  }}1,
+  // CHECK-FIXES: {{^                  }}2,
+  // CHECK-FIXES: {{^                  }}3,
+  // CHECK-FIXES: {{^                  }}4
+  // CHECK-FIXES: {{^          }}};
+  // CHECK-FIXES: {{^  }}int bb = 4;
+  // clang-format on
+
+  TemT *T1 = &TT1, *T2 = &TT2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: TemT *T1 = &TT1;
+  // CHECK-FIXES: {{^  }}TemT *T2 = &TT2;
+
+  const PointerType *PT1 = T1->getAs<PointerType>(),
+                    *PT2 = T2->getAs<PointerType>();
+  // CHECK-MESSAGES: [[@LINE-2]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: const PointerType *PT1 = T1->getAs<PointerType>();
+  // CHECK-FIXES: {{^  }}const PointerType *PT2 = T2->getAs<PointerType>();
+
+  const int *p1 = nullptr;
+  const int *p2 = nullptr;
+
+  const int *&pref1 = p1, *&pref2 = p2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: const int *&pref1 = p1;
+  // CHECK-FIXES: {{^  }}const int *&pref2 = p2;
+
+  // clang-format off
+  const char *literal1 = "clang"   "test"\
+                         "one",
+             *literal2 = "empty", literal3[] = "three";
+  // CHECK-MESSAGES: [[@LINE-3]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: const char *literal1 = "clang"   "test"\
+  // CHECK-FIXES: {{^                         }}"one";
+  // CHECK-FIXES: {{^  }}const char *literal2 = "empty";
+  // CHECK-FIXES: {{^  }}const char literal3[] = "three";
+  // clang-format on
+}
+
+void g() try {
+  int i, j;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: int i;
+  // CHECK-FIXES: {{^  }}int j;
+} catch (...) {
+}
+
+struct S {
+  int a;
+  const int b;
+  void f() {}
+};
+
+void memberPointers() {
+  typedef const int S::*MemPtr;
+  MemPtr aaa = &S::a, bbb = &S::b;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-FIXES: MemPtr aaa = &S::a;
+  // CHECK-FIXES: {{^  }}MemPtr bbb = &S::b;
+}
+
+typedef int *tptr, tbt;
+typedef int (&tfp)(int, long), tarr[10];
+typedef int tarr2[10], tct;
+
+template <typename A, typename B>
+void should_not_be_touched(A, B);
+
+int variable, function(void);
+
+int call_func_with_sideeffect();
+void bad_if_decl() {
+  if (true)
+    int i, j, k = call_func_with_sideeffect();
+}