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

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk added inline comments.


================
Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:23
+      memberExpr(hasDeclaration(anyOf(cxxMethodDecl(isStaticStorageClass()),
+                                      varDecl(hasStaticStorageDuration()))),
+                 unless(isInTemplateInstantiation()))
----------------
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.


https://reviews.llvm.org/D35937