[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Jeff Trull via Phabricator via cfe-commits]

jaafar added inline comments.



Comment at: cfe/trunk/lib/Format/TokenAnnotator.cpp:3072
+  return false;
+if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
+  return true;

This is the clause that triggers the problem.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Jeff Trull via Phabricator via cfe-commits]

jaafar added a comment.

It's been pointed out to me that 28546 preceded this commit... so it may only 
//appear// related.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Jeff Trull via Phabricator via cfe-commits]

jaafar added a comment.

Hi everyone, I've been investigating a bug and bisected it to this commit.

The problem, in brief, is that lambdas passed as arguments can cause an extra 
line break before the first argument, but only if the lambda is neither the 
first nor the last argument. This implies, of course, that there are at least 
three arguments supplied... Perhaps the most minimal example is:

  void f()
{
func(0, [] {}, 0);
}

which gets formatted as:

  void
  f ()
  {
  func (
  0, [] {}, 0);
  }

(note the unnecessary newline after the left paren)

I found three existing reports of this problem: 28546 
,  45141 
, and 45424 
. I'm posting here in lieu of 
creating a fourth duplicate bug report, in the hopes that it will be more 
likely to find someone who can fix it. Thank you.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52676



___
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

[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

[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

[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

[PATCH] D81769: 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:38
 enum BindArgumentKind { BK_Temporary, BK_Placeholder, BK_CallExpr, BK_Other };
-enum CaptureMode { CM_None, CM_ByRef, CM_ByValue, CM_InitExpression };
+enum CaptureMode { CM_None, CM_ByRef, CM_ByValue };
+enum CaptureExpr { CE_None, CE_Var, CE_InitExpression };

"Captured by reference" and "uses init expression" are not mutually exclusive - 
that gave rise to item 6 above.



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

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.



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"

Is there a better way to express this? "a single child of type ThisExpr" was 
what I was going for...


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: Repair various issues with modernize-avoid-bind

[Jeff Trull via Phabricator via cfe-commits]

jaafar created this revision.
jaafar added reviewers: aaron.ballman, alexfh, hokein.
jaafar added a project: clang-tools-extra.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

In the process of running this check on a large codebase 
 I found a number of limitations, and 
thought I would pass on my fixes for possible integration upstream:

1. Templated function call operators are not supported
2. Function object constructors are always used directly in the lambda body, 
even if their arguments are not captured
3. Placeholders with namespace qualifiers (`std::placeholders::_1`) are not 
detected
4. Lambda arguments should be forwarded to the stored function
5. Data members from other classes still get captured with `this`
6. Expressions (as opposed to variables) inside `std::ref` are not captured 
properly
7. Function object templates sometimes have their template arguments replaced 
with concrete types

This patch resolves all those issues and adds suitable unit tests.

If desired, I can separate these commits out into 7 separate diffs, but it 
seemed like it might be easier to evaluate them all at once.


Repository:
  rG LLVM Github Monorepo

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 

[PATCH] D81530: [clangd] Log rather than assert on bad UTF-8.

[Jeff Trull via Phabricator via cfe-commits]

jaafar added a comment.

Thanks for the detailed analysis! I have filed 
https://github.com/boostorg/spirit/issues/612


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81530



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

[PATCH] D81530: [clangd] Log rather than assert on bad UTF-8.

[Jeff Trull via Phabricator via cfe-commits]

jaafar added a comment.

I can confirm that this commit works equally well for the UTF-8 assertion 
failure. Thank you!

Is there some way to easily modify the source to produce a diagnostic pointing 
to the issue's source location? I would like to file an appropriate bug in 
Boost.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81530



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

[PATCH] D74025: [clangd] Add the missing elaborated types in FindTarget.

[Jeff Trull via Phabricator via cfe-commits]

jaafar added a comment.

Thnaks very much @hokein . The ones I need are:
9a5c448a31bacc08e73fcae4636094f9b6e2be6a 

eaf0c89ec5f866b6cef296c542c030bb2cf8481d 

d66afd6dde542dc373f87e07fe764c071fe20d76 

as well as this patch, under review but dormant since February: 
https://reviews.llvm.org/D74731

With these fixes I have a pleasant experience on my large codebases. Thanks for 
your help.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74025



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

[PATCH] D74025: [clangd] Add the missing elaborated types in FindTarget.

[Jeff Trull via Phabricator via cfe-commits]

jaafar added a comment.

Thank you. I'm trying to balance stability with getting the fixes I need and so 
have been avoiding the "bleeding edge". I find that unfortunately the newest 
code tends to be "unstable", as those releases are rightfully named, and I need 
clangd for my daily work.

There are three commits, including this one, without which clangd cannot run 
for more than 10 or 20 seconds. They have all been in the codebase for several 
months now.  I would be happy to provide a list. Thanks again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74025



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

[PATCH] D74025: [clangd] Add the missing elaborated types in FindTarget.

[Jeff Trull via Phabricator via cfe-commits]

jaafar added a comment.

I'm very happy this fix exists. I see it's in master, but not in 10.0.0 or 
10.0.1-rc1 either. Is there any chance it can be released?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74025



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