[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!



================
Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:23
+      memberExpr(hasDeclaration(anyOf(cxxMethodDecl(isStaticStorageClass()),
+                                      varDecl(hasStaticStorageDuration()))),
+                 unless(isInTemplateInstantiation()))
----------------
barancsuk wrote:
> aaron.ballman wrote:
> > barancsuk wrote:
> > > aaron.ballman wrote:
> > > > Why not use `isStaticStorageClass()` here as well?
> > > Unfortunately, `isStaticStorageClass()` misses variable declarations that do not contain the static keyword, including definitions of static variables outside of their class.
> > > However, `hasStaticStorageDuration()` has no problem finding all static variable declarations correctly. 
> > Under what circumstances would that make a difference? If the variable is declared within the class with the static storage specifier, that declaration should be sufficient for the check, regardless of how the definition looks, no?
> > 
> > If it does make a difference, can you add a test case that demonstrates that?
> `isStaticStorageClass()` does fail for the current test cases.
> 
> The reason for that is the function `hasDecl()`, that, for static variables with multiple declarations,  may return a declaration that does not contain the `static` keyword.
> For example, it finds `int C::x = 0;` rather than `static int x;`
> Then `isStaticStorageClass()` discards it.
> 
> However, `hasStaticStorageDuration()` works fine, because all declarations are static storage duration, regardless of whether the `static` keyword is present or not.
Hmmm, that's unexpected (though not incorrect, from looking at the matcher implementations). Thank you for the explanation, I think it's fine for your patch.


https://reviews.llvm.org/D35937