This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

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

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
kbobyrev added inline comments.Sep 20 2018, 9:00 AM
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 updated this revision to Diff 167270.Sep 27 2018, 4:02 AM
JonasToth marked 6 inline comments as done.
  • 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 updated this revision to Diff 167282.Sep 27 2018, 4:58 AM
JonasToth marked 3 inline comments as done.
  • add space after clang-tidy in banner
  • refactor slightly, add better in-code comments to explain whats happening, add more TODOs and FIXMEs

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.

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 added inline comments.Sep 27 2018, 7:45 AM
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 added inline comments.Sep 27 2018, 8:08 AM
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 added inline comments.Sep 27 2018, 8:31 AM
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.

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 added inline comments.Sep 27 2018, 8:58 AM
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 added inline comments.Sep 27 2018, 9:01 AM
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 marked 11 inline comments as done.Sep 27 2018, 11:45 AM
JonasToth added inline comments.
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).

  • 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 marked 9 inline comments as done.
  • 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 marked 9 inline comments as done.Sep 27 2018, 12:46 PM
JonasToth added inline comments.
clang-tidy/readability/IsolateDeclCheck.cpp
212

I prefer this one :)

JonasToth marked 2 inline comments as done.Sep 27 2018, 12:47 PM
JonasToth updated this revision to Diff 167389.Sep 27 2018, 1:32 PM
  • bail out in transformation if there are PP directives in the source range of the DeclStmt
JonasToth updated this revision to Diff 167397.Sep 27 2018, 1:47 PM
  • fix doc typo
JonasToth marked an inline comment as done.Sep 27 2018, 1:56 PM
JonasToth updated this revision to Diff 167400.Sep 27 2018, 1:56 PM
  • last cleanup for today, testing on clang/lib looks good, but is already very clean
JonasToth updated this revision to Diff 167437.Sep 28 2018, 1:49 AM
  • write user-facing documentation
JonasToth retitled this revision from [WIP][clang-tidy] initial ideas to isolate variable declarations to [clang-tidy] new check 'readability-isolate-declaration'.Sep 28 2018, 1:54 AM
JonasToth edited the summary of this revision. (Show Details)

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.

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 added inline comments.Sep 28 2018, 2:08 AM
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 ;)

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 added inline comments.Sep 28 2018, 2:13 AM
test/clang-tidy/readability-isolate-decl.cpp
185
JonasToth marked 3 inline comments as done.Sep 28 2018, 2:18 AM
JonasToth updated this revision to Diff 167445.Sep 28 2018, 2:28 AM
  • add tests for auto and decltype
  • rename check to 'readability-isolate-declaration'

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.

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.

Blender looks better now, code compiles after full transformation

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)

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

JonasToth updated this revision to Diff 167491.Sep 28 2018, 8:59 AM
  • fix paren issue, introduced non-local changes i will extract and post in a separate patch
  • remove debug output for paren search
JonasToth added inline comments.Sep 28 2018, 9:02 AM
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

jsji removed a subscriber: jsji.Sep 28 2018, 9:11 AM

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 added inline comments.Sep 28 2018, 11:07 AM
clang-tidy/readability/IsolateDeclarationCheck.cpp
230 ↗(On Diff #167491)

Use of auto?

250 ↗(On Diff #167491)

Excepts -> Accepts

291 ↗(On Diff #167491)

Debugging code that can be removed.

clang-tidy/utils/LexerUtils.cpp
89

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

clang-tidy/utils/LexerUtils.h
49

No real need for this local.

How about creating CERT alias?

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

It'll be better to compare with 0 explicitly.

145 ↗(On Diff #167491)

auto could be used here.

198 ↗(On Diff #167491)

Will be good idea to use range loop.

docs/ReleaseNotes.rst
60

Do we really need this entry?

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

Because the decrement is post-fix it will decrement past 0 on the breaking condition.
Having unsigned will result in a wrap (is defined, but still slightly non-obvious).

I could make a do - while, then the condition can be --Indirections != 0. I would just like to follow the CPPCG that say 'don't use unsigned unless you need modulo arithmetic'.

230 ↗(On Diff #167491)

I can use CharSourceRange too. kbobryev wanted auto :)

clang-tidy/utils/LexerUtils.cpp
89

In the context of the check it is expected that this range is valid and all tokens can be found. I adjusted the naming of the function to better reflect the purpose.

clang-tidy/utils/LexerUtils.h
49

I found it unexpected, that failure is signaled with true, so for readability I added this longer tmp variable. Are there other ways to signal true == failure?

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

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

30 ↗(On Diff #168091)

Maybe inline this variable?

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

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

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

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

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

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

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

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

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

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

// TODO: Handle FieldDecl's as well

Mostly minor nits that clean up wording and comments.

clang-tidy/readability/IsolateDeclarationCheck.cpp
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
90

Tok->is()

clang-tidy/utils/LexerUtils.h
78

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

81

I don't think the functionality is obvious from the name -- this is testing to see whether any token in the given range is either a macro or a preprocessor directive. How about reversing the logic to: rangeContainsExpansionsOrDirectives()

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

Typo. declarationaration -> declaration

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

10 ↗(On Diff #169389)

linebreak -> line break

I think it may also be important to point out that the declarations will remain in the same order as their original source order. For instance, you may run into code like: int i = 5, j = i; and it's crucial that it be transformed into int i = 5; int j = i;.

23 ↗(On Diff #169389)

does exclude -> excludes
commong -> common

31 ↗(On Diff #169389)

undesired in it's own -> undesirable

45 ↗(On Diff #169389)

Why are global variables excluded from this check? It seems like they should have the exact same behavior as local variables in terms of the transformation, no?

63 ↗(On Diff #169389)

Add a comma after "Furthermore".

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

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

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

That was definitly too deep in the night :D

45 ↗(On Diff #169389)

I thought so too, but take a look at the AST:

int global_i, global_j = 42, global_k;
void f() {
    int local_i, local_j = 42, local_k;
}
|-VarDecl 0x55c428c4ab08 </home/jonas/Programme/test/test_decls_global.cpp:1:1, col:5> col:5 global_i 'int'
|-VarDecl 0x55c428c4abc0 <col:1, col:26> col:15 global_j 'int' cinit
| `-IntegerLiteral 0x55c428c4ac20 <col:26> 'int' 42
|-VarDecl 0x55c428c4ac58 <col:1, col:30> col:30 global_k 'int'
`-FunctionDecl 0x55c428c4ad30 <line:2:1, line:4:1> line:2:6 f 'void ()'
  `-CompoundStmt 0x55c428c4af90 <col:10, line:4:1>
    `-DeclStmt 0x55c428c4af78 <line:3:5, col:39>
      |-VarDecl 0x55c428c4ade8 <col:5, col:9> col:9 local_i 'int'
      |-VarDecl 0x55c428c4ae60 <col:5, col:28> col:18 local_j 'int' cinit
      | `-IntegerLiteral 0x55c428c4aec0 <col:28> 'int' 42
      `-VarDecl 0x55c428c4aef8 <col:5, col:32> col:32 local_k 'int'

The locals create a DeclStmt and the globals don't, right now the transformation depends on the fact that its a DeclStmt, similar to class members that are FieldDecl.

I did short investigation on the issue, but couldn't think of a good way to fix that issue. I was not capable of "grouping" these decls even though they are together. Maybe I just missed something obvious, but right it's not supported. I would love to actually support it tough.

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

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

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

Mark DeclCount as const

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

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

ZaMaZaN4iK added inline comments.

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

+ if (DeclCount < 2)

Mark DeclCount as const

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D51949

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

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

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

LGTM!

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

Yeah, this looks like a deficiency with the AST representation, to me. It doesn't need to be solved for this patch, but it may be an interesting next step for the check for a future patch.

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