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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.

Sorry, it does not appear that i'm being useful in this review.




Comment at: 
clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp:89-101
+  isDefinition(), isUserProvided(), 
unless(isExpansionInSystemHeader()),
+  unless(isVirtual()), unless(isStatic()), unless(hasTrivialBody()),
+  unless(isOverloadedOperator()), unless(isConstructor()),
+  unless(isDestructor()), unless(isConversionOperator()),
+  unless(isTemplate()), unless(isDependentContext()),
+  unless(ofClass(
+  anyOf(isLambda(),

I'm pretty sure you can do
```
unless(anyOf(isExpansionInSystemHeader(), isVirtual(), ...))
```



Comment at: clang-tools-extra/test/clang-tidy/readability-static-method.cpp:103
+}
+
+template 

Is there a test where a lambda (variant with and without capturing this, either 
explicitly, or implicitly)
is within a non-static class function?



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

[PATCH] D61749: [clang-tidy] initial version of readability-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(
+"readability-static-method");
 CheckFactories.registerCheck(

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(Node);
+}
+
+AST_MATCHER(CXXMethodDecl, isDestructor) {
+  return isa(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