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