[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:23
+      memberExpr(hasDeclaration(anyOf(cxxMethodDecl(isStaticStorageClass()),
+                                      varDecl(hasStaticStorageDuration()))),
+                 unless(isInTemplateInstantiation()))
----------------
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?


================
Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:30
+    return NameSpecifierNestingLevel;
+  } else
+    return 0;
----------------
No `else` after a `return`.


================
Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:79
+  if (BaseExpr->HasSideEffects(*AstContext) ||
+      (getNameSpecifierNestingLevel(BaseType) > NameSpecifierNestingThreshold))
+    return;
----------------
No need for the extra parens here.


================
Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.h:20
+/// \@brief Checks for member expressions that access static members through instances
+/// and replaces them with calls to the preferred, more readable `::` operator.
+///
----------------
aaron.ballman wrote:
> `::` isn't an operator -- I would say: "and replaces them with uses of the appropriate qualified-id."
Reflow the comments.


================
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;{{$}}
+}
----------------
barancsuk wrote:
> 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.
> Maybe for now, the check should filter these expressions with side effects.
> They might better be addressed in a follow-up patch.

I think that's a good approach.


================
Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:90
+
+  // u.u is not changed to N::V::T::U::u; because the nesting level is over 3.
+  u.u = 12;
----------------
It'd be good to have an additional RUN line that sets the nesting level to something other than the default and ensure the behavior is consistent.


https://reviews.llvm.org/D35937