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.

cfe-commits mailing list

Reply via email to