Re: [PATCH] D24652: [clang-tidy] readability-avoid-const-params-in-decls template instantiation bugfix

[Alexander Kornienko via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

LG


https://reviews.llvm.org/D24652



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24652: [clang-tidy] readability-avoid-const-params-in-decls template instantiation bugfix

[Malcolm Parsons via cfe-commits]

malcolm.parsons added inline comments.


Comment at: clang-tidy/readability/AvoidConstParamsInDecls.cpp:39
@@ -38,2 +38,3 @@
// generate a non-definition FunctionDecl too. Ignore those.
-   unless(cxxMethodDecl(ofClass(cxxRecordDecl(isLambda(),
+   // Ignore template instantiations too.
+   unless(cxxMethodDecl(ofClass(cxxRecordDecl(anyOf(

alexfh wrote:
> Would be helpful to expand on why we're ignoring only member function of 
> class instantiations (and not instantiations of member or free standing 
> functions, for example).
AFAICT, template instantiations of member and free standing functions are 
definitions when the template is a definition.


https://reviews.llvm.org/D24652



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24652: [clang-tidy] readability-avoid-const-params-in-decls template instantiation bugfix

[Malcolm Parsons via cfe-commits]

malcolm.parsons updated this revision to Diff 72476.
malcolm.parsons added a comment.

Expand code comment


https://reviews.llvm.org/D24652

Files:
  clang-tidy/readability/AvoidConstParamsInDecls.cpp
  test/clang-tidy/readability-avoid-const-params-in-decls.cpp

Index: test/clang-tidy/readability-avoid-const-params-in-decls.cpp
===
--- test/clang-tidy/readability-avoid-const-params-in-decls.cpp
+++ test/clang-tidy/readability-avoid-const-params-in-decls.cpp
@@ -53,6 +53,12 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: parameter 'b'
 // CHECK-FIXES: void F12(bool b = true);
 
+template
+int F13(const bool b = true);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'b'
+// CHECK-FIXES: int F13(bool b = true);
+int f13 = F13();
+
 struct Foo {
   Foo(const int i);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: parameter 'i'
@@ -63,6 +69,18 @@
   // CHECK-FIXES: void operator()(int i);
 };
 
+template 
+struct FooT {
+  FooT(const int i);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: parameter 'i'
+  // CHECK-FIXES: FooT(int i);
+
+  void operator()(const int i);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: parameter 'i'
+  // CHECK-FIXES: void operator()(int i);
+};
+FooT f(1);
+
 // Do not match on definitions
 void NF1(const int i) {}
 void NF2(const int *const i) {}
@@ -72,6 +90,25 @@
 void NF6(const int *const) {}
 void NF7(int, const int) {}
 void NF8(const int, const int) {}
+template 
+int NF9(const int, const int) { return 0; }
+int nf9 = NF9(1, 2);
+
+// Do not match on inline member functions
+struct Bar {
+  Bar(const int i) {}
+
+  void operator()(const int i) {}
+};
+
+// Do not match on inline member functions of a templated class
+template 
+struct BarT {
+  BarT(const int i) {}
+
+  void operator()(const int i) {}
+};
+BarT b(1);
 
 // Do not match on other stuff
 void NF(const alias_type& i);
Index: clang-tidy/readability/AvoidConstParamsInDecls.cpp
===
--- clang-tidy/readability/AvoidConstParamsInDecls.cpp
+++ clang-tidy/readability/AvoidConstParamsInDecls.cpp
@@ -36,7 +36,12 @@
   functionDecl(unless(isDefinition()),
// Lambdas are always their own definition, but they
// generate a non-definition FunctionDecl too. Ignore those.
-   unless(cxxMethodDecl(ofClass(cxxRecordDecl(isLambda(),
+   // Class template instantiations have a non-definition
+   // CXXMethodDecl for methods that aren't used in this
+   // translation unit. Ignore those, as the template will have
+   // already been checked.
+   unless(cxxMethodDecl(ofClass(cxxRecordDecl(anyOf(
+   isLambda(), 
ast_matchers::isTemplateInstantiation()),
has(typeLoc(forEach(ConstParamDecl
   .bind("func"),
   this);


Index: test/clang-tidy/readability-avoid-const-params-in-decls.cpp
===
--- test/clang-tidy/readability-avoid-const-params-in-decls.cpp
+++ test/clang-tidy/readability-avoid-const-params-in-decls.cpp
@@ -53,6 +53,12 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: parameter 'b'
 // CHECK-FIXES: void F12(bool b = true);
 
+template
+int F13(const bool b = true);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'b'
+// CHECK-FIXES: int F13(bool b = true);
+int f13 = F13();
+
 struct Foo {
   Foo(const int i);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: parameter 'i'
@@ -63,6 +69,18 @@
   // CHECK-FIXES: void operator()(int i);
 };
 
+template 
+struct FooT {
+  FooT(const int i);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: parameter 'i'
+  // CHECK-FIXES: FooT(int i);
+
+  void operator()(const int i);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: parameter 'i'
+  // CHECK-FIXES: void operator()(int i);
+};
+FooT f(1);
+
 // Do not match on definitions
 void NF1(const int i) {}
 void NF2(const int *const i) {}
@@ -72,6 +90,25 @@
 void NF6(const int *const) {}
 void NF7(int, const int) {}
 void NF8(const int, const int) {}
+template 
+int NF9(const int, const int) { return 0; }
+int nf9 = NF9(1, 2);
+
+// Do not match on inline member functions
+struct Bar {
+  Bar(const int i) {}
+
+  void operator()(const int i) {}
+};
+
+// Do not match on inline member functions of a templated class
+template 
+struct BarT {
+  BarT(const int i) {}
+
+  void operator()(const int i) {}
+};
+BarT b(1);
 
 // Do not match on other stuff
 void NF(const alias_type& i);
Index: clang-tidy/readability/AvoidConstParamsInDecls.cpp
===
--- clang-tidy/readability/AvoidConstParamsInDecls.cpp
+++ clang-tidy/readability/AvoidConstParamsInDecls.cpp
@@ -36,7 +36,12 @@
   functionDecl(unless(isDefinition()),
// Lambdas are always their own definition, but 

Re: [PATCH] D24652: [clang-tidy] readability-avoid-const-params-in-decls template instantiation bugfix

[Alexander Kornienko via cfe-commits]

alexfh requested changes to this revision.
This revision now requires changes to proceed.


Comment at: clang-tidy/readability/AvoidConstParamsInDecls.cpp:39
@@ -38,2 +38,3 @@
// generate a non-definition FunctionDecl too. Ignore those.
-   unless(cxxMethodDecl(ofClass(cxxRecordDecl(isLambda(),
+   // Ignore template instantiations too.
+   unless(cxxMethodDecl(ofClass(cxxRecordDecl(anyOf(

Would be helpful to expand on why we're ignoring only member function of class 
instantiations (and not instantiations of member or free standing functions, 
for example).


https://reviews.llvm.org/D24652



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24652: [clang-tidy] readability-avoid-const-params-in-decls template instantiation bugfix

[Kirill Bobyrev via cfe-commits]

omtcyfz added inline comments.


Comment at: clang-tidy/readability/AvoidConstParamsInDecls.cpp:41
@@ -40,1 +40,3 @@
+   unless(cxxMethodDecl(ofClass(cxxRecordDecl(anyOf(
+   isLambda(), 
ast_matchers::isTemplateInstantiation()),
has(typeLoc(forEach(ConstParamDecl

malcolm.parsons wrote:
> omtcyfz wrote:
> > `ast_matchers::` is redundant here.
> You'd think so, but it didn't compile.
Ah, I see.

Also, please provide more info next time:

> it didn't compile

doesn't give anything.

For the others: the actual problem is that 
`llvm/tools/clang/include/clang/Basic/Specifiers.h` has `inline bool 
isTemplateInstantiation(TemplateSpecializationKind Kind)` function. It might 
make sense to change name of the AST Matcher to something else in order to 
prevent collision. Thoughts, suggestions?

However, this, of course, is not in scope of the current patch.


https://reviews.llvm.org/D24652



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24652: [clang-tidy] readability-avoid-const-params-in-decls template instantiation bugfix

[Malcolm Parsons via cfe-commits]

malcolm.parsons added inline comments.


Comment at: clang-tidy/readability/AvoidConstParamsInDecls.cpp:41
@@ -40,1 +40,3 @@
+   unless(cxxMethodDecl(ofClass(cxxRecordDecl(anyOf(
+   isLambda(), 
ast_matchers::isTemplateInstantiation()),
has(typeLoc(forEach(ConstParamDecl

omtcyfz wrote:
> `ast_matchers::` is redundant here.
You'd think so, but it didn't compile.


https://reviews.llvm.org/D24652



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24652: [clang-tidy] readability-avoid-const-params-in-decls template instantiation bugfix

[Kirill Bobyrev via cfe-commits]

omtcyfz added a subscriber: omtcyfz.
omtcyfz added a comment.

Probably it also makes sense to reflect both `lambda` and template 
instantiation parts in documentation, since I find current wording totally 
confusing at the moment.



Comment at: clang-tidy/readability/AvoidConstParamsInDecls.cpp:41
@@ -40,1 +40,3 @@
+   unless(cxxMethodDecl(ofClass(cxxRecordDecl(anyOf(
+   isLambda(), 
ast_matchers::isTemplateInstantiation()),
has(typeLoc(forEach(ConstParamDecl

`ast_matchers::` is redundant here.


https://reviews.llvm.org/D24652



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits