[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:
> 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