[PATCH] D81769: [clang-tidy] Repair various issues with modernize-avoid-bind

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

Thank you for the patch! I've commit on your behalf in 
95a3550dc89a0d424d90e2c0ad30d9ecfa9422cf 



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81769/new/

https://reviews.llvm.org/D81769



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

[PATCH] D81769: [clang-tidy] Repair various issues with modernize-avoid-bind

[Jeff Trull via Phabricator via cfe-commits]

jaafar added a comment.

I do need someone to commit this, yes, thank you.

I'm Jeff Trull edas...@att.net


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81769/new/

https://reviews.llvm.org/D81769



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

[PATCH] D81769: [clang-tidy] Repair various issues with modernize-avoid-bind

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D81769#2109936 , @jaafar wrote:

> One more simplification from Aaron. Thanks!


No problem! Do you need someone to commit on your behalf, btw? If so, let me 
know what name and email address you would like me to attribute.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81769/new/

https://reviews.llvm.org/D81769



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

[PATCH] D81769: [clang-tidy] Repair various issues with modernize-avoid-bind

[Jeff Trull via Phabricator via cfe-commits]

jaafar updated this revision to Diff 272813.
jaafar marked an inline comment as done.
jaafar added a comment.

One more simplification from Aaron. Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81769/new/

https://reviews.llvm.org/D81769

Files:
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
@@ -7,11 +7,11 @@
 
 template 
 bind_rt bind(Fp &&, Arguments &&...);
-}
+} // namespace impl
 
 template 
 T ref(T );
-}
+} // namespace std
 
 namespace boost {
 template 
@@ -58,12 +58,33 @@
 
 void UseF(F);
 
+struct G {
+  G() : _member(0) {}
+  G(int m) : _member(m) {}
+
+  template 
+  void operator()(T) const {}
+
+  int _member;
+};
+
+template 
+struct H {
+  void operator()(T) const {};
+};
+
 struct placeholder {};
 placeholder _1;
 placeholder _2;
 
+namespace placeholders {
+using ::_1;
+using ::_2;
+} // namespace placeholders
+
 int add(int x, int y) { return x + y; }
 int addThree(int x, int y, int z) { return x + y + z; }
+void sub(int , int y) { x += y; }
 
 // Let's fake a minimal std::function-like facility.
 namespace std {
@@ -107,6 +128,7 @@
   int MemberVariable;
   static int StaticMemberVariable;
   F MemberStruct;
+  G MemberStructWithData;
 
   void testCaptureByValue(int Param, F f) {
 int x = 3;
@@ -145,6 +167,11 @@
 auto GGG = boost::bind(UseF, MemberStruct);
 // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to boost::bind [modernize-avoid-bind]
 // CHECK-FIXES: auto GGG = [this] { return UseF(MemberStruct); };
+
+auto HHH = std::bind(add, MemberStructWithData._member, 1);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
+// Correctly distinguish data members of other classes
+// CHECK-FIXES: auto HHH = [capture0 = MemberStructWithData._member] { return add(capture0, 1); };
   }
 };
 
@@ -217,17 +244,38 @@
   auto EEE = std::bind(*D::create(), 1, 2);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
   // CHECK-FIXES: auto EEE = [Func = *D::create()] { return Func(1, 2); };
+
+  auto FFF = std::bind(G(), 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // Templated function call operators may be used
+  // CHECK-FIXES: auto FFF = [] { return G()(1); };
+
+  int CTorArg = 42;
+  auto GGG = std::bind(G(CTorArg), 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // Function objects with constructor arguments should be captured
+  // CHECK-FIXES: auto GGG = [Func = G(CTorArg)] { return Func(1); };
 }
 
+template 
+void testMemberFnOfClassTemplate(T) {
+  auto HHH = std::bind(H(), 42);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // Ensure function class template arguments are preserved
+  // CHECK-FIXES: auto HHH = [] { return H()(42); };
+}
+
+template void testMemberFnOfClassTemplate(int);
+
 void testPlaceholders() {
   int x = 2;
   auto AAA = std::bind(add, x, _1);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto AAA = [x](auto && PH1) { return add(x, PH1); };
+  // CHECK-FIXES: auto AAA = [x](auto && PH1) { return add(x, std::forward(PH1)); };
 
   auto BBB = std::bind(add, _2, _1);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto BBB = [](auto && PH1, auto && PH2) { return add(PH2, PH1); };
+  // CHECK-FIXES: auto BBB = [](auto && PH1, auto && PH2) { return add(std::forward(PH2), std::forward(PH1)); };
 
   // No fix is applied for reused placeholders.
   auto CCC = std::bind(add, _1, _1);
@@ -238,7 +286,12 @@
   // unnamed parameters.
   auto DDD = std::bind(add, _2, 1);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto DDD = [](auto &&, auto && PH2) { return add(PH2, 1); };
+  // CHECK-FIXES: auto DDD = [](auto &&, auto && PH2) { return add(std::forward(PH2), 1); };
+
+  // Namespace-qualified placeholders are valid too
+  auto EEE = std::bind(add, placeholders::_2, 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // CHECK-FIXES: auto EEE = [](auto &&, auto && PH2) { return add(std::forward(PH2), 1); };
 }
 
 void testGlobalFunctions() {
@@ -267,6 +320,7 @@
 void testCapturedSubexpressions() {
   int x = 3;
   int y = 3;
+  int *p = 
 
   auto AAA = std::bind(add, 1, add(2, 5));
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
@@ -277,6 +331,11 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 

[PATCH] D81769: [clang-tidy] Repair various issues with modernize-avoid-bind

[Aaron Ballman via Phabricator via cfe-commits]

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

Aside from a minor nit, this LGTM




Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:414-415
+  for (const clang::Decl *D : RecordDecl->decls()) {
+if (D->getKind() != Decl::Kind::FunctionTemplate)
+  continue;
+

You can drop this entirely -- the `dyn_cast<>` below does the work for you.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81769/new/

https://reviews.llvm.org/D81769



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

[PATCH] D81769: [clang-tidy] Repair various issues with modernize-avoid-bind

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:243
+  if ((std::distance(ME->child_begin(), ME->child_end()) == 1) &&
+  isa(*ME->children().begin())) {
+// reference to data member without explicit "this"

jaafar wrote:
> aaron.ballman wrote:
> > jaafar wrote:
> > > Is there a better way to express this? "a single child of type ThisExpr" 
> > > was what I was going for...
> > `if (isa(ME->getBase()))` -- `MemberExpr::children()` only 
> > considers the base anyway, so there's only ever one child node to begin 
> > with.
> Could there be zero? I'm just worried about dereferencing 
> `ME->children().begin()` before verifying there is at least one...
> 
The child nodes in `MemberExpr` just point to one node, its base, which should 
never be null. Is it not simpler to just use this?
```
if (isa(ME->getBase())) {
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81769/new/

https://reviews.llvm.org/D81769



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

[PATCH] D81769: [clang-tidy] Repair various issues with modernize-avoid-bind

[Jeff Trull via Phabricator via cfe-commits]

jaafar updated this revision to Diff 272674.
jaafar added a comment.

Applied feedback from Nathan James


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81769/new/

https://reviews.llvm.org/D81769

Files:
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
@@ -7,11 +7,11 @@
 
 template 
 bind_rt bind(Fp &&, Arguments &&...);
-}
+} // namespace impl
 
 template 
 T ref(T );
-}
+} // namespace std
 
 namespace boost {
 template 
@@ -58,12 +58,33 @@
 
 void UseF(F);
 
+struct G {
+  G() : _member(0) {}
+  G(int m) : _member(m) {}
+
+  template 
+  void operator()(T) const {}
+
+  int _member;
+};
+
+template 
+struct H {
+  void operator()(T) const {};
+};
+
 struct placeholder {};
 placeholder _1;
 placeholder _2;
 
+namespace placeholders {
+using ::_1;
+using ::_2;
+} // namespace placeholders
+
 int add(int x, int y) { return x + y; }
 int addThree(int x, int y, int z) { return x + y + z; }
+void sub(int , int y) { x += y; }
 
 // Let's fake a minimal std::function-like facility.
 namespace std {
@@ -107,6 +128,7 @@
   int MemberVariable;
   static int StaticMemberVariable;
   F MemberStruct;
+  G MemberStructWithData;
 
   void testCaptureByValue(int Param, F f) {
 int x = 3;
@@ -145,6 +167,11 @@
 auto GGG = boost::bind(UseF, MemberStruct);
 // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to boost::bind [modernize-avoid-bind]
 // CHECK-FIXES: auto GGG = [this] { return UseF(MemberStruct); };
+
+auto HHH = std::bind(add, MemberStructWithData._member, 1);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
+// Correctly distinguish data members of other classes
+// CHECK-FIXES: auto HHH = [capture0 = MemberStructWithData._member] { return add(capture0, 1); };
   }
 };
 
@@ -217,17 +244,38 @@
   auto EEE = std::bind(*D::create(), 1, 2);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
   // CHECK-FIXES: auto EEE = [Func = *D::create()] { return Func(1, 2); };
+
+  auto FFF = std::bind(G(), 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // Templated function call operators may be used
+  // CHECK-FIXES: auto FFF = [] { return G()(1); };
+
+  int CTorArg = 42;
+  auto GGG = std::bind(G(CTorArg), 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // Function objects with constructor arguments should be captured
+  // CHECK-FIXES: auto GGG = [Func = G(CTorArg)] { return Func(1); };
 }
 
+template 
+void testMemberFnOfClassTemplate(T) {
+  auto HHH = std::bind(H(), 42);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // Ensure function class template arguments are preserved
+  // CHECK-FIXES: auto HHH = [] { return H()(42); };
+}
+
+template void testMemberFnOfClassTemplate(int);
+
 void testPlaceholders() {
   int x = 2;
   auto AAA = std::bind(add, x, _1);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto AAA = [x](auto && PH1) { return add(x, PH1); };
+  // CHECK-FIXES: auto AAA = [x](auto && PH1) { return add(x, std::forward(PH1)); };
 
   auto BBB = std::bind(add, _2, _1);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto BBB = [](auto && PH1, auto && PH2) { return add(PH2, PH1); };
+  // CHECK-FIXES: auto BBB = [](auto && PH1, auto && PH2) { return add(std::forward(PH2), std::forward(PH1)); };
 
   // No fix is applied for reused placeholders.
   auto CCC = std::bind(add, _1, _1);
@@ -238,7 +286,12 @@
   // unnamed parameters.
   auto DDD = std::bind(add, _2, 1);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto DDD = [](auto &&, auto && PH2) { return add(PH2, 1); };
+  // CHECK-FIXES: auto DDD = [](auto &&, auto && PH2) { return add(std::forward(PH2), 1); };
+
+  // Namespace-qualified placeholders are valid too
+  auto EEE = std::bind(add, placeholders::_2, 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // CHECK-FIXES: auto EEE = [](auto &&, auto && PH2) { return add(std::forward(PH2), 1); };
 }
 
 void testGlobalFunctions() {
@@ -267,6 +320,7 @@
 void testCapturedSubexpressions() {
   int x = 3;
   int y = 3;
+  int *p = 
 
   auto AAA = std::bind(add, 1, add(2, 5));
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
@@ -277,6 +331,11 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
   // Results of 

[PATCH] D81769: [clang-tidy] Repair various issues with modernize-avoid-bind

[Jeff Trull via Phabricator via cfe-commits]

jaafar marked 3 inline comments as done.
jaafar added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:243
+  if ((std::distance(ME->child_begin(), ME->child_end()) == 1) &&
+  isa(*ME->children().begin())) {
+// reference to data member without explicit "this"

njames93 wrote:
> jaafar wrote:
> > aaron.ballman wrote:
> > > jaafar wrote:
> > > > Is there a better way to express this? "a single child of type 
> > > > ThisExpr" was what I was going for...
> > > `if (isa(ME->getBase()))` -- `MemberExpr::children()` only 
> > > considers the base anyway, so there's only ever one child node to begin 
> > > with.
> > Could there be zero? I'm just worried about dereferencing 
> > `ME->children().begin()` before verifying there is at least one...
> > 
> The child nodes in `MemberExpr` just point to one node, its base, which 
> should never be null. Is it not simpler to just use this?
> ```
> if (isa(ME->getBase())) {
> ```
Ah, much better, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81769/new/

https://reviews.llvm.org/D81769



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

[PATCH] D81769: [clang-tidy] Repair various issues with modernize-avoid-bind

[Jeff Trull via Phabricator via cfe-commits]

jaafar updated this revision to Diff 272580.
jaafar added a comment.

Applied feedback from Aaron Ballman. Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81769/new/

https://reviews.llvm.org/D81769

Files:
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
@@ -7,11 +7,11 @@
 
 template 
 bind_rt bind(Fp &&, Arguments &&...);
-}
+} // namespace impl
 
 template 
 T ref(T );
-}
+} // namespace std
 
 namespace boost {
 template 
@@ -58,12 +58,33 @@
 
 void UseF(F);
 
+struct G {
+  G() : _member(0) {}
+  G(int m) : _member(m) {}
+
+  template 
+  void operator()(T) const {}
+
+  int _member;
+};
+
+template 
+struct H {
+  void operator()(T) const {};
+};
+
 struct placeholder {};
 placeholder _1;
 placeholder _2;
 
+namespace placeholders {
+using ::_1;
+using ::_2;
+} // namespace placeholders
+
 int add(int x, int y) { return x + y; }
 int addThree(int x, int y, int z) { return x + y + z; }
+void sub(int , int y) { x += y; }
 
 // Let's fake a minimal std::function-like facility.
 namespace std {
@@ -107,6 +128,7 @@
   int MemberVariable;
   static int StaticMemberVariable;
   F MemberStruct;
+  G MemberStructWithData;
 
   void testCaptureByValue(int Param, F f) {
 int x = 3;
@@ -145,6 +167,11 @@
 auto GGG = boost::bind(UseF, MemberStruct);
 // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to boost::bind [modernize-avoid-bind]
 // CHECK-FIXES: auto GGG = [this] { return UseF(MemberStruct); };
+
+auto HHH = std::bind(add, MemberStructWithData._member, 1);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
+// Correctly distinguish data members of other classes
+// CHECK-FIXES: auto HHH = [capture0 = MemberStructWithData._member] { return add(capture0, 1); };
   }
 };
 
@@ -217,17 +244,38 @@
   auto EEE = std::bind(*D::create(), 1, 2);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
   // CHECK-FIXES: auto EEE = [Func = *D::create()] { return Func(1, 2); };
+
+  auto FFF = std::bind(G(), 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // Templated function call operators may be used
+  // CHECK-FIXES: auto FFF = [] { return G()(1); };
+
+  int CTorArg = 42;
+  auto GGG = std::bind(G(CTorArg), 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // Function objects with constructor arguments should be captured
+  // CHECK-FIXES: auto GGG = [Func = G(CTorArg)] { return Func(1); };
 }
 
+template 
+void testMemberFnOfClassTemplate(T) {
+  auto HHH = std::bind(H(), 42);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // Ensure function class template arguments are preserved
+  // CHECK-FIXES: auto HHH = [] { return H()(42); };
+}
+
+template void testMemberFnOfClassTemplate(int);
+
 void testPlaceholders() {
   int x = 2;
   auto AAA = std::bind(add, x, _1);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto AAA = [x](auto && PH1) { return add(x, PH1); };
+  // CHECK-FIXES: auto AAA = [x](auto && PH1) { return add(x, std::forward(PH1)); };
 
   auto BBB = std::bind(add, _2, _1);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto BBB = [](auto && PH1, auto && PH2) { return add(PH2, PH1); };
+  // CHECK-FIXES: auto BBB = [](auto && PH1, auto && PH2) { return add(std::forward(PH2), std::forward(PH1)); };
 
   // No fix is applied for reused placeholders.
   auto CCC = std::bind(add, _1, _1);
@@ -238,7 +286,12 @@
   // unnamed parameters.
   auto DDD = std::bind(add, _2, 1);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto DDD = [](auto &&, auto && PH2) { return add(PH2, 1); };
+  // CHECK-FIXES: auto DDD = [](auto &&, auto && PH2) { return add(std::forward(PH2), 1); };
+
+  // Namespace-qualified placeholders are valid too
+  auto EEE = std::bind(add, placeholders::_2, 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // CHECK-FIXES: auto EEE = [](auto &&, auto && PH2) { return add(std::forward(PH2), 1); };
 }
 
 void testGlobalFunctions() {
@@ -267,6 +320,7 @@
 void testCapturedSubexpressions() {
   int x = 3;
   int y = 3;
+  int *p = 
 
   auto AAA = std::bind(add, 1, add(2, 5));
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
@@ -277,6 +331,11 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
   // 

[PATCH] D81769: [clang-tidy] Repair various issues with modernize-avoid-bind

[Jeff Trull via Phabricator via cfe-commits]

jaafar marked an inline comment as done.
jaafar added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:243
+  if ((std::distance(ME->child_begin(), ME->child_end()) == 1) &&
+  isa(*ME->children().begin())) {
+// reference to data member without explicit "this"

aaron.ballman wrote:
> jaafar wrote:
> > Is there a better way to express this? "a single child of type ThisExpr" 
> > was what I was going for...
> `if (isa(ME->getBase()))` -- `MemberExpr::children()` only 
> considers the base anyway, so there's only ever one child node to begin with.
Could there be zero? I'm just worried about dereferencing 
`ME->children().begin()` before verifying there is at least one...



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81769/new/

https://reviews.llvm.org/D81769



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

[PATCH] D81769: [clang-tidy] Repair various issues with modernize-avoid-bind

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D81769#2106574 , @jaafar wrote:

> "Ping". I hope this can be considered for 10.0.1. If nothing else I think 
> reviewing the test cases has a lot of value - there are some real issues with 
> the current checks and fixits.


Thank you for all of these fixes! I like the direction it's going in, but am a 
bit wary of introducing this for 10.0.1 because it changes a lot of behaviors 
(not to say it shouldn't be 10.0.1, just to express a bit of trepidation).




Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:150
 
+static bool tryCaptureAsLocalVariable(const MatchFinder::MatchResult ,
+  BindArgument , const Expr *E);

jaafar wrote:
> These forward declarations seemed to make the diffs a lot easier to read. 
> It's also possible to move the two functions into this position, of course.
I think it's fine to use forward declares, but I'd prefer them just after the 
anonymous namespace (or before it) so the declarations are towards the top of 
the file instead of stuck in the middle.



Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:166
+} else {
+  // the argument to std::ref is an expression that produces a reference
+  // create a capture reference to hold it

Comments should be grammatical, so include capitalization and punctuation. 
(Here and elsewhere in the patch.)



Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:243
+  if ((std::distance(ME->child_begin(), ME->child_end()) == 1) &&
+  isa(*ME->children().begin())) {
+// reference to data member without explicit "this"

jaafar wrote:
> Is there a better way to express this? "a single child of type ThisExpr" was 
> what I was going for...
`if (isa(ME->getBase()))` -- `MemberExpr::children()` only 
considers the base anyway, so there's only ever one child node to begin with.



Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:282
 SmallVector Matches;
-if (MatchPlaceholder.match(B.SourceTokens, )) {
+clang::DeclRefExpr const *DRE = dyn_cast(E);
+if (MatchPlaceholder.match(B.SourceTokens, ) ||

`const auto *` since the type is spelled out in the initialization.



Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:414-418
+if (D->getKind() != Decl::Kind::FunctionTemplate)
+  continue;
+
+const clang::FunctionTemplateDecl *FT =
+clang::dyn_cast(D);

This doesn't need to be done in two steps. Better:
```
const auto *FTD = dyn_cast(D);
if (!FTD)
  continue;

const FunctionDecl *FD = FTD->getTemplatedDecl();
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81769/new/

https://reviews.llvm.org/D81769



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

[PATCH] D81769: [clang-tidy] Repair various issues with modernize-avoid-bind

[Jeff Trull via Phabricator via cfe-commits]

jaafar added a comment.

"Ping". I hope this can be considered for 10.0.1. If nothing else I think 
reviewing the test cases has a lot of value - there are some real issues with 
the current checks and fixits.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81769/new/

https://reviews.llvm.org/D81769



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