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