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

[Matthias Gehre via Phabricator via cfe-commits]

mgehre marked 2 inline comments as done.
mgehre added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp:96
+    CheckFactories.registerCheck<StaticMethodCheck>(
+        "readability-static-method");
     CheckFactories.registerCheck<StringCompareCheck>(
----------------
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`?


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


================
Comment at: clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp:28-31
+  bool VisitCXXThisExpr(const CXXThisExpr *E) {
+    Used = true;
+    return false; // Stop traversal.
+  }
----------------
aaron.ballman wrote:
> 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.
I added a test


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61749