[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()))
----------------
Why not use `isStaticStorageClass()` here as well?


================
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.
+///
----------------
`::` isn't an operator -- I would say: "and replaces them with uses of the appropriate qualified-id."


================
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.
+
----------------
Eugene.Zelenko wrote:
> Is :: operator really?
Same wording suggestion here as above.


================
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;{{$}}
+}
----------------
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.


================
Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:137
+  h<4>();
+}
----------------
Please add some tests where the replacement is somewhat more complex, like:
```
namespace N {
struct S {
  struct T {
    struct U {
      static int x;
     };
  };
};
}

// Complex replacement
void f(N::S::T::U u) {
  u.x = 12; // N::S::T::U::x = 12;
}
```
Note that this is a case where it's possible that the code becomes *less* readable rather than more readable. I am fine without having any configuration right now, but we may want to consider whether the nesting level should factor in to whether we think this is more or less readable.


Repository:
  rL LLVM

https://reviews.llvm.org/D35937