[PATCH] D115106: [clang-tidy] Fix `readability-static-accessed-through-instance` false negative for static methods

[Simon Giesecke via Phabricator via cfe-commits]

simon.giesecke added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6017
+/// cxxMethodDecl(isStatic()) matches A::foo() but not A::bar()
+AST_MATCHER(CXXMethodDecl, isStatic) { return Node.isStatic(); }
+
----------------
aaron.ballman wrote:
> I would prefer we not expose this AST matcher as a public one. There's sufficient confusion around "as written in source" vs "computed" vs "linkage" in this area that I think we should be careful when introducing matchers around storage classes and storage durations. Then again, the water is already pretty muddy here, so maybe this ship sailed...
> 
> Other potential solutions to this issue would be to expose an AST matcher to traverse to the canonical decl or only matches the canonical decl, then we could use that to wrap the existing call to `isStaticStorageClass()`. (Of course, we could make this matcher local to just that source file, as well.)
> 
> Thoughts?
I think if we have a `isStaticStorageClass` matcher, then we can also have `isStatic` matcher.

But maybe we shouldn't have either of those in the public header, but maybe add a different one as you suggested, under a suitable name with as little ambiguity as possible.

And maybe keep the `isStaticStorageClass` matcher but rename it to reduce the chance of misunderstanding?

Then, as mentioned in my original top-level comment, I can imagine that there are more uses of the `isStaticStorageClass` matcher in clang-tidy rules (and maybe elsewhere) that actually should use the new matcher.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115106/new/

https://reviews.llvm.org/D115106