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