[PATCH] D61749: [clang-tidy] initial version of readability-const-method

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp:96
+    CheckFactories.registerCheck<StaticMethodCheck>(
+        "readability-static-method");
     CheckFactories.registerCheck<StringCompareCheck>(
----------------
mgehre wrote:
> aaron.ballman wrote:
> > I'm not super keen on the check name. It's not very descriptive -- does this operate on static methods, does it make methods static, does it turn global dynamic initialization into static method initialization?
> > 
> > How about: `readability-convert-functions-to-static` or something more descriptive of the functionality?
> I agree that we should find a better name. With `readability-convert-functions-to-static`, I miss the difference between adding `static` linkage to a free function versus making a member function static.
> How about `readability-convert-member-functions-to-static` or `readability-convert-method-to-static`?
I think `readability-convert-member-functions-to-static` would be a good name.


================
Comment at: clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp:136-149
+  if (Definition->isConst()) {
+    // Make sure that we either remove 'const' on both declaration and
+    // definition or emit no fix-it at all.
+    SourceRange DefConst = getLocationOfConst(Definition->getTypeSourceInfo(),
+                                       *Result.SourceManager,
+                                       Result.Context->getLangOpts());
+
----------------
mgehre wrote:
> aaron.ballman wrote:
> > `const` isn't the only thing to worry about though, no? You need to handle things like:
> > ```
> > struct S {
> >   void f() volatile;
> >   void g() &;
> >   void h() &&;
> > };
> > ```
> > Another one I am curious about are computed noexcept specifications and whether those are handled properly. e.g.,
> > ```
> > struct S {
> >   int i;
> >   void foo(SomeClass C) noexcept(noexcept(C + i));
> > };
> > ```
> I added tests for those cases and disabled fix-it generation for them to keep the code simple (for now).
There's an obscure edge case missing:
```
struct S {
  void f() __restrict;
};
```


================
Comment at: clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp:35-45
+AST_MATCHER(CXXMethodDecl, isConstructor) {
+  return isa<CXXConstructorDecl>(Node);
+}
+
+AST_MATCHER(CXXMethodDecl, isDestructor) {
+  return isa<CXXDestructorDecl>(Node);
+}
----------------
Is there a reason we can't use `cxxConstructorDecl()`, `cxxDestructorDecl()`, and `cxxConversionDecl()` instead?


================
Comment at: clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp:130
+  StringRef Text = getStringFromRange(SourceMgr, LangOpts, Range);
+  size_t Offset = Text.find("const");
+  if (Offset == StringRef::npos)
----------------
What does this do for code like:
```
#define constantly_annoying volatile

struct S {
  void func() constantly_annoying {}
};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61749