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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
