[PATCH] D61749: [clang-tidy] initial version of readability-static-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>(
----------------
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?


================
Comment at: clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp:25
+public:
+  FindUsageOfThis() {}
+  bool Used = false;
----------------
Do you need this constructor at all? If so, `= default;` it.


================
Comment at: clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp:28-31
+  bool VisitCXXThisExpr(const CXXThisExpr *E) {
+    Used = true;
+    return false; // Stop traversal.
+  }
----------------
Does this find a usage of `this` in unevaluated contexts? e.g.,
```
struct S {
  size_t derp() { return sizeof(this); }
};
```
I am hoping the answer is yes, because we wouldn't want to convert that into a static function. Probably worth a test.


================
Comment at: clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp:56-57
+                                      const LangOptions &LangOpts) {
+  if (!TSI)
+    return {};
+  FunctionTypeLoc FTL =
----------------
Under what circumstances can this happen?


================
Comment at: clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp:58
+    return {};
+  FunctionTypeLoc FTL =
+      TSI->getTypeLoc().IgnoreParens().getAs<FunctionTypeLoc>();
----------------
Can use `auto` here.


================
Comment at: clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp:63
+
+  auto Range = SourceRange{FTL.getRParenLoc().getLocWithOffset(1),
+                           FTL.getLocalRangeEnd()};
----------------
`SourceRange Range{...};`


================
Comment at: clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp:68
+  StringRef Text = getStringFromRange(SourceMgr, LangOpts, Range);
+  auto Offset = Text.find("const");
+  if (Offset == StringRef::npos)
----------------
Don't use `auto` here.


================
Comment at: clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp:87-108
+  if (!Definition->isUserProvided())
+    return;
+  if (Definition->isStatic())
+    return; // Nothing we can improve.
+  if (Definition->isVirtual())
+    return;
+  if (Definition->getParent()->hasAnyDependentBases())
----------------
Much of this logic should be hoisted into the AST matcher code rather than handled one-off here. Some of it can be done directly, others may require new local matchers to be defined.


================
Comment at: clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp:116
+
+  const CXXMethodDecl* Declaration = Definition->getCanonicalDecl();
+
----------------
Formatting is wrong here -- run the patch through clang-format.


================
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());
+
----------------
`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));
};
```


================
Comment at: clang-tools-extra/test/clang-tidy/readability-static-method.cpp:99
+void KeepLambdas() {
+  auto F = +[]() { return 0; };
+  auto F2 = []() { return 0; };
----------------
This is too subtle for my tastes. Another way to trigger the conversion is a type cast, which is far more understandable to anyone reading the code a few years from now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61749