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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
