barancsuk added inline comments.
================ Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:23 + memberExpr(hasDeclaration(anyOf(cxxMethodDecl(isStaticStorageClass()), + varDecl(hasStaticStorageDuration()))), + unless(isInTemplateInstantiation())) ---------------- 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. ================ Comment at: docs/clang-tidy/checks/readability-static-accessed-through-instance.rst:7 +Checks for member expressions that access static members through instances, and +replaces them with the corresponding expressions that use a more readable `::` operator. + ---------------- aaron.ballman wrote: > Eugene.Zelenko wrote: > > Is :: operator really? > Same wording suggestion here as above. The suggested wording is indeed a clearer one, and I rewrote the descriptions and the documentation accordingly, however I found, that in the C++ International Standard `::` is referred to as a scope (resolution) operator. ================ Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:34 + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through instance [readability-static-accessed-through-instance] + // CHECK-FIXES: {{^}} C::x;{{$}} +} ---------------- aaron.ballman wrote: > baloghadamsoftware wrote: > > xazax.hun wrote: > > > baloghadamsoftware wrote: > > > > aaron.ballman wrote: > > > > > This fix-it worries me because it changes the semantics of the code. > > > > > The function `f()` is no longer called, and so this isn't a valid > > > > > source transformation. > > > > Maybe the correct fix would be here f(1, 2, 3, 4); C::x; > > > Maybe for now we should just skip this cases. Expr::HasSideEffects might > > > be a good candidate to filter these cases. > > I think including the expression with side effect before the member access > > (as I suggested) is not more complicated than skipping these cases. > Please ensure you handle the more complex cases then, such as: > ``` > struct S { > static int X; > }; > > void g(int &); > S h(); > > void f(S s) { > g(h().X); > if ([s]() { return s; }().X == 15) > ; > } > ``` Expressions with side effects introduce some troublesome cases. For example, in the following code, the static member expression, `h().x` does not get evaluated if a() returns false. ``` struct C { static int x; }; void j(int); int k(bool); bool a(); C h(); void g(){ j(k(a() && h().x)); } ``` Maybe for now, the check should filter these expressions with side effects. They might better be addressed in a follow-up 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