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