[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check
This revision was automatically updated to reflect the committed changes.
Closed by commit rG64f74bf72eb4: [clang-tidy] Rewrite modernize-avoid-bind
check. (authored by zturner).
Changed prior to commit:
https://reviews.llvm.org/D70368?vs=230661=231793#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70368/new/
https://reviews.llvm.org/D70368
Files:
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst
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
@@ -8,75 +8,62 @@
template
bind_rt bind(Fp &&, Arguments &&...);
}
+
+template
+T ref(T );
}
-int add(int x, int y) { return x + y; }
+namespace boost {
+template
+class bind_rt {};
-void f() {
- auto clj = std::bind(add, 2, 2);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind]
- // CHECK-FIXES: auto clj = [] { return add(2, 2); };
-}
+template
+bind_rt bind(const Fp &, Arguments...);
-void g() {
- int x = 2;
- int y = 2;
- auto clj = std::bind(add, x, y);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [=] { return add(x, y); };
+template
+struct reference_wrapper {
+ explicit reference_wrapper(T ) {}
+};
+
+template
+reference_wrapper const ref(T ) {
+ return reference_wrapper(t);
}
-struct placeholder {};
-placeholder _1;
-placeholder _2;
+} // namespace boost
-void h() {
- int x = 2;
- auto clj = std::bind(add, x, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); };
-}
+namespace C {
+int add(int x, int y) { return x + y; }
+} // namespace C
-struct A;
-struct B;
-bool ABTest(const A &, const B &);
+struct Foo {
+ static int add(int x, int y) { return x + y; }
+};
-void i() {
- auto BATest = std::bind(ABTest, _2, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); };
-}
+struct D {
+ D() = default;
+ void operator()(int x, int y) const {}
-void j() {
- auto clj = std::bind(add, 2, 2, 2);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for argument mismatches.
- // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2);
-}
+ void MemberFunction(int x) {}
-void k() {
- auto clj = std::bind(add, _1, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for reused placeholders.
- // CHECK-FIXES: auto clj = std::bind(add, _1, _1);
-}
+ static D *create();
+};
-void m() {
- auto clj = std::bind(add, 1, add(2, 5));
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for nested calls.
- // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5));
-}
+struct F {
+ F(int x) {}
+ ~F() {}
-namespace C {
- int add(int x, int y){ return x + y; }
-}
+ int get() { return 42; }
+};
-void n() {
- auto clj = std::bind(C::add, 1, 1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [] { return C::add(1, 1); };
-}
+void UseF(F);
+
+struct placeholder {};
+placeholder _1;
+placeholder _2;
+
+int add(int x, int y) { return x + y; }
+int addThree(int x, int y, int z) { return x + y + z; }
// Let's fake a minimal std::function-like facility.
namespace std {
@@ -114,10 +101,213 @@
void Reset(std::function);
};
-void test(Thing *t) {
+int GlobalVariable = 42;
+
+struct TestCaptureByValueStruct {
+ int MemberVariable;
+ static int StaticMemberVariable;
+ F MemberStruct;
+
+ void testCaptureByValue(int Param, F f) {
+int x = 3;
+int y = 4;
+auto AAA = std::bind(add, x, y);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto AAA = [x, y] { return add(x, y); };
+
+// When the captured variable is repeated, it should only appear in the capture list once.
+auto BBB = std::bind(add, x, x);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto BBB = [x] { return add(x, x); };
+
+int LocalVariable;
+// Global variables shouldn't be captured at all, and members should be captured through this.
+
[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check
aaron.ballman accepted this revision.
aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM once you handle the last remaining nits from @Eugene.Zelenko.
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:116
+static const Expr *ignoreTemporariesAndImplicitCasts(const Expr *E) {
+ if (const auto *T = dyn_cast(E))
+return ignoreTemporariesAndImplicitCasts(T->GetTemporaryExpr());
zturner wrote:
> aaron.ballman wrote:
> > What about `CXXBindTemporaryExpr`?
> >
> > Would `Expr::IgnoreImplicit()` do too much stripping because it removes
> > `FullExpr` as well?
> I don't actually know. This patch is literally my first exposure to clang's
> frontend and AST manipulations, so I was kind of just trying different things
> until something worked here. I didn't even know about `IgnoreImplicit()` or
> `FullExpr`. I'll play around with them and report back.
Unless you find some reason why they're critical in the initial version of your
patch, I'm fine handling this as a follow-up if needed.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70368/new/
https://reviews.llvm.org/D70368
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check
Eugene.Zelenko added a comment. Please mark addressed comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70368/new/ https://reviews.llvm.org/D70368 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check
Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:194 + The check now supports supports diagnosing and fixing arbitrary callables instead of + only simple free functions. The ``PermissiveParameterList`` option has also been + added to address situations where the existing fix-it logic would sometimes generate Please use single back-tics for options. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst:50 + If the option is set to non-zero, the check will append ``auto&&...`` to the end + of every placeholder parameter list. Without this, it is possible for a fixit + to perform an incorrect transformation in the case where the result of the ``bind`` Double space here and in other places are still not fixed. Same for fixit. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70368/new/ https://reviews.llvm.org/D70368 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check
zturner updated this revision to Diff 230661.
zturner added a comment.
Addressed suggestions from @Eugene.Zelenko
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70368/new/
https://reviews.llvm.org/D70368
Files:
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst
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
@@ -8,75 +8,62 @@
template
bind_rt bind(Fp &&, Arguments &&...);
}
+
+template
+T ref(T );
}
-int add(int x, int y) { return x + y; }
+namespace boost {
+template
+class bind_rt {};
-void f() {
- auto clj = std::bind(add, 2, 2);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind]
- // CHECK-FIXES: auto clj = [] { return add(2, 2); };
-}
+template
+bind_rt bind(const Fp &, Arguments...);
-void g() {
- int x = 2;
- int y = 2;
- auto clj = std::bind(add, x, y);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [=] { return add(x, y); };
+template
+struct reference_wrapper {
+ explicit reference_wrapper(T ) {}
+};
+
+template
+reference_wrapper const ref(T ) {
+ return reference_wrapper(t);
}
-struct placeholder {};
-placeholder _1;
-placeholder _2;
+} // namespace boost
-void h() {
- int x = 2;
- auto clj = std::bind(add, x, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); };
-}
+namespace C {
+int add(int x, int y) { return x + y; }
+} // namespace C
-struct A;
-struct B;
-bool ABTest(const A &, const B &);
+struct Foo {
+ static int add(int x, int y) { return x + y; }
+};
-void i() {
- auto BATest = std::bind(ABTest, _2, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); };
-}
+struct D {
+ D() = default;
+ void operator()(int x, int y) const {}
-void j() {
- auto clj = std::bind(add, 2, 2, 2);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for argument mismatches.
- // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2);
-}
+ void MemberFunction(int x) {}
-void k() {
- auto clj = std::bind(add, _1, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for reused placeholders.
- // CHECK-FIXES: auto clj = std::bind(add, _1, _1);
-}
+ static D *create();
+};
-void m() {
- auto clj = std::bind(add, 1, add(2, 5));
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for nested calls.
- // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5));
-}
+struct F {
+ F(int x) {}
+ ~F() {}
-namespace C {
- int add(int x, int y){ return x + y; }
-}
+ int get() { return 42; }
+};
-void n() {
- auto clj = std::bind(C::add, 1, 1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [] { return C::add(1, 1); };
-}
+void UseF(F);
+
+struct placeholder {};
+placeholder _1;
+placeholder _2;
+
+int add(int x, int y) { return x + y; }
+int addThree(int x, int y, int z) { return x + y + z; }
// Let's fake a minimal std::function-like facility.
namespace std {
@@ -114,10 +101,213 @@
void Reset(std::function);
};
-void test(Thing *t) {
+int GlobalVariable = 42;
+
+struct TestCaptureByValueStruct {
+ int MemberVariable;
+ static int StaticMemberVariable;
+ F MemberStruct;
+
+ void testCaptureByValue(int Param, F f) {
+int x = 3;
+int y = 4;
+auto AAA = std::bind(add, x, y);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto AAA = [x, y] { return add(x, y); };
+
+// When the captured variable is repeated, it should only appear in the capture list once.
+auto BBB = std::bind(add, x, x);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto BBB = [x] { return add(x, x); };
+
+int LocalVariable;
+// Global variables shouldn't be captured at all, and members should be captured through this.
+auto CCC = std::bind(add, MemberVariable, GlobalVariable);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+//
[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check
Eugene.Zelenko added a comment. Changes should be also reflected in Release Notes. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst:7 +The check finds uses of ``std::bind`` and ``boost::bind`` and replaces them +with lambdas. Lambdas will use value-capture unless reference capture is +explicitly requested with ``std::ref`` or ``boost::ref``. Please fix double space. Same in other places. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst:12 +and free functions, and all variations thereof. Anything that you can pass +to the first argument of `bind` should be diagnosable. Currently, the only +known case where a fixit is unsupported is when the same placeholder is Please use double back-ticks for bind. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst:13 +to the first argument of `bind` should be diagnosable. Currently, the only +known case where a fixit is unsupported is when the same placeholder is +specified multiple times in the parameter list. fixit -> fix-it. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst:49 + + If the option is set to non-zero, the check will append `auto&&...` to the end + of every placeholder parameter list. Without this, it is possible for a fixit Please use double back-ticks for auto&& Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst:50 + If the option is set to non-zero, the check will append `auto&&...` to the end + of every placeholder parameter list. Without this, it is possible for a fixit + to perform an incorrect transformation in the case where the result of the bind fixit -> fix-it. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst:51 + of every placeholder parameter list. Without this, it is possible for a fixit + to perform an incorrect transformation in the case where the result of the bind + is used in the context of a type erased functor such as ``std::function`` which Please use double back-ticks for bind. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst:64 + +is valid code, and returns `4`. The actual values passed to `ignore_args` are +simply ignored. Without `PermissiveParameterList`, this would be transformed into Please use double back-ticks for ignore_args. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst:75 + +which will *not* compile, since the lambda does not contain an `operator()` that +that accepts 2 arguments. With permissive parameter list, it instead generates Please use double back-ticks for operator(). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70368/new/ https://reviews.llvm.org/D70368 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check
zturner updated this revision to Diff 230495.
zturner added a comment.
- Updated documentation for this check
- Incorporated additional suggestions from @aaron.ballman
- Fixed an invalid transformation that was generated when binding a member
function and the second argument of `bind` (the object pointer) was a
placeholder. Test is added for this case as well.
- Fixed an invalid transformation that was generated when a placeholder index
was entirely skipped, as in the call `std::bind(add, 0, _2);` In this case we
need to generate an unused placeholder in the first position of the resulting
lambda's parameter list.
- Added a clang-tidy option `PermissiveParameterList` which appends `auto&&...`
to the end of every lambda's placeholder list. This is necessary in some
situations to prevent clang-tidy from applying a fixit that causes the code to
no longer compile.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70368/new/
https://reviews.llvm.org/D70368
Files:
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h
clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst
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
@@ -8,75 +8,62 @@
template
bind_rt bind(Fp &&, Arguments &&...);
}
+
+template
+T ref(T );
}
-int add(int x, int y) { return x + y; }
+namespace boost {
+template
+class bind_rt {};
-void f() {
- auto clj = std::bind(add, 2, 2);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind]
- // CHECK-FIXES: auto clj = [] { return add(2, 2); };
-}
+template
+bind_rt bind(const Fp &, Arguments...);
-void g() {
- int x = 2;
- int y = 2;
- auto clj = std::bind(add, x, y);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [=] { return add(x, y); };
+template
+struct reference_wrapper {
+ explicit reference_wrapper(T ) {}
+};
+
+template
+reference_wrapper const ref(T ) {
+ return reference_wrapper(t);
}
-struct placeholder {};
-placeholder _1;
-placeholder _2;
+} // namespace boost
-void h() {
- int x = 2;
- auto clj = std::bind(add, x, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); };
-}
+namespace C {
+int add(int x, int y) { return x + y; }
+} // namespace C
-struct A;
-struct B;
-bool ABTest(const A &, const B &);
+struct Foo {
+ static int add(int x, int y) { return x + y; }
+};
-void i() {
- auto BATest = std::bind(ABTest, _2, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); };
-}
+struct D {
+ D() = default;
+ void operator()(int x, int y) const {}
-void j() {
- auto clj = std::bind(add, 2, 2, 2);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for argument mismatches.
- // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2);
-}
+ void MemberFunction(int x) {}
-void k() {
- auto clj = std::bind(add, _1, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for reused placeholders.
- // CHECK-FIXES: auto clj = std::bind(add, _1, _1);
-}
+ static D *create();
+};
-void m() {
- auto clj = std::bind(add, 1, add(2, 5));
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for nested calls.
- // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5));
-}
+struct F {
+ F(int x) {}
+ ~F() {}
-namespace C {
- int add(int x, int y){ return x + y; }
-}
+ int get() { return 42; }
+};
-void n() {
- auto clj = std::bind(C::add, 1, 1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [] { return C::add(1, 1); };
-}
+void UseF(F);
+
+struct placeholder {};
+placeholder _1;
+placeholder _2;
+
+int add(int x, int y) { return x + y; }
+int addThree(int x, int y, int z) { return x + y + z; }
// Let's fake a minimal std::function-like facility.
namespace std {
@@ -114,10 +101,213 @@
void Reset(std::function);
};
-void test(Thing *t) {
+int GlobalVariable = 42;
+
+struct TestCaptureByValueStruct {
+ int MemberVariable;
+ static int StaticMemberVariable;
+ F MemberStruct;
+
+ void testCaptureByValue(int Param, F f) {
+int x = 3;
+int y = 4;
+auto AAA = std::bind(add, x, y);
+// CHECK-MESSAGES: :[[@LINE-1]]:16:
[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check
aaron.ballman added inline comments.
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:153
+
+ return HNM.matchesNode(*dyn_cast(D));
+}
zturner wrote:
> aaron.ballman wrote:
> > This should probably use `cast<>` if it's going to assume the returned
> > value is never null.
> Good point. Since we're talking about this code anyway, it felt super hacky
> to instantiate an AST matcher just to check for the qualified name of a Decl.
> Is there a better way to do this?
Since you only care about named call declarations, I think you could probably
get away with:
```
if (const auto *ND = dyn_cast(CE->getCalleeDecl())) {
const std::string = ND->getQualifiedNameAsString();
if (Str == "::boost::ref" || Str == "::std::ref") {
...
}
}
```
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:167
+}
+ } else if (const auto *ThisExpr = dyn_cast(Statement))
+return true;
`isa(Statement)`
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:428-429
+if (const auto *DRE = dyn_cast(CallExpression)) {
+ if (const auto *FD = dyn_cast(DRE->getDecl()))
+return FD;
+}
I think this can be replaced with: `return
dyn_cast(DRE->getDecl());`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70368/new/
https://reviews.llvm.org/D70368
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check
zturner updated this revision to Diff 230162.
zturner added a comment.
Forgot to remove spurious `llvm::` namespace qualifications.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70368/new/
https://reviews.llvm.org/D70368
Files:
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h
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
@@ -8,75 +8,51 @@
template
bind_rt bind(Fp &&, Arguments &&...);
}
+
+template
+T ref(T );
}
-int add(int x, int y) { return x + y; }
+namespace boost {
+template
+class bind_rt {};
-void f() {
- auto clj = std::bind(add, 2, 2);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind]
- // CHECK-FIXES: auto clj = [] { return add(2, 2); };
-}
+template
+bind_rt bind(const Fp &, Arguments...);
+} // namespace boost
-void g() {
- int x = 2;
- int y = 2;
- auto clj = std::bind(add, x, y);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [=] { return add(x, y); };
-}
+namespace C {
+int add(int x, int y) { return x + y; }
+} // namespace C
-struct placeholder {};
-placeholder _1;
-placeholder _2;
+struct Foo {
+ static int add(int x, int y) { return x + y; }
+};
-void h() {
- int x = 2;
- auto clj = std::bind(add, x, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); };
-}
+struct D {
+ D() = default;
+ void operator()(int x, int y) const {}
-struct A;
-struct B;
-bool ABTest(const A &, const B &);
+ void MemberFunction(int x) {}
-void i() {
- auto BATest = std::bind(ABTest, _2, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); };
-}
+ static D *create();
+};
-void j() {
- auto clj = std::bind(add, 2, 2, 2);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for argument mismatches.
- // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2);
-}
+struct F {
+ F(int x) {}
+ ~F() {}
-void k() {
- auto clj = std::bind(add, _1, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for reused placeholders.
- // CHECK-FIXES: auto clj = std::bind(add, _1, _1);
-}
+ int get() { return 42; }
+};
-void m() {
- auto clj = std::bind(add, 1, add(2, 5));
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for nested calls.
- // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5));
-}
+void UseF(F);
-namespace C {
- int add(int x, int y){ return x + y; }
-}
+struct placeholder {};
+placeholder _1;
+placeholder _2;
-void n() {
- auto clj = std::bind(C::add, 1, 1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [] { return C::add(1, 1); };
-}
+int add(int x, int y) { return x + y; }
+int addThree(int x, int y, int z) { return x + y + z; }
// Let's fake a minimal std::function-like facility.
namespace std {
@@ -114,10 +90,188 @@
void Reset(std::function);
};
-void test(Thing *t) {
+int GlobalVariable = 42;
+
+struct TestCaptureByValueStruct {
+ int MemberVariable;
+ static int StaticMemberVariable;
+ F MemberStruct;
+
+ void testCaptureByValue(int Param, F f) {
+int x = 3;
+int y = 4;
+auto AAA = std::bind(add, x, y);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto AAA = [x, y] { return add(x, y); };
+
+// When the captured variable is repeated, it should only appear in the capture list once.
+auto BBB = std::bind(add, x, x);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto BBB = [x] { return add(x, x); };
+
+int LocalVariable;
+// Global variables shouldn't be captured at all, and members should be captured through this.
+auto CCC = std::bind(add, MemberVariable, GlobalVariable);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto CCC = [this] { return add(MemberVariable, GlobalVariable); };
+
+// Static member variables shouldn't be captured, but locals should
+auto DDD = std::bind(add, TestCaptureByValueStruct::StaticMemberVariable, LocalVariable);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+//
[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check
zturner updated this revision to Diff 230161.
zturner added a comment.
Updated with suggestions from @aaron.ballman
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70368/new/
https://reviews.llvm.org/D70368
Files:
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h
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
@@ -8,75 +8,51 @@
template
bind_rt bind(Fp &&, Arguments &&...);
}
+
+template
+T ref(T );
}
-int add(int x, int y) { return x + y; }
+namespace boost {
+template
+class bind_rt {};
-void f() {
- auto clj = std::bind(add, 2, 2);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind]
- // CHECK-FIXES: auto clj = [] { return add(2, 2); };
-}
+template
+bind_rt bind(const Fp &, Arguments...);
+} // namespace boost
-void g() {
- int x = 2;
- int y = 2;
- auto clj = std::bind(add, x, y);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [=] { return add(x, y); };
-}
+namespace C {
+int add(int x, int y) { return x + y; }
+} // namespace C
-struct placeholder {};
-placeholder _1;
-placeholder _2;
+struct Foo {
+ static int add(int x, int y) { return x + y; }
+};
-void h() {
- int x = 2;
- auto clj = std::bind(add, x, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); };
-}
+struct D {
+ D() = default;
+ void operator()(int x, int y) const {}
-struct A;
-struct B;
-bool ABTest(const A &, const B &);
+ void MemberFunction(int x) {}
-void i() {
- auto BATest = std::bind(ABTest, _2, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); };
-}
+ static D *create();
+};
-void j() {
- auto clj = std::bind(add, 2, 2, 2);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for argument mismatches.
- // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2);
-}
+struct F {
+ F(int x) {}
+ ~F() {}
-void k() {
- auto clj = std::bind(add, _1, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for reused placeholders.
- // CHECK-FIXES: auto clj = std::bind(add, _1, _1);
-}
+ int get() { return 42; }
+};
-void m() {
- auto clj = std::bind(add, 1, add(2, 5));
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for nested calls.
- // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5));
-}
+void UseF(F);
-namespace C {
- int add(int x, int y){ return x + y; }
-}
+struct placeholder {};
+placeholder _1;
+placeholder _2;
-void n() {
- auto clj = std::bind(C::add, 1, 1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [] { return C::add(1, 1); };
-}
+int add(int x, int y) { return x + y; }
+int addThree(int x, int y, int z) { return x + y + z; }
// Let's fake a minimal std::function-like facility.
namespace std {
@@ -114,10 +90,188 @@
void Reset(std::function);
};
-void test(Thing *t) {
+int GlobalVariable = 42;
+
+struct TestCaptureByValueStruct {
+ int MemberVariable;
+ static int StaticMemberVariable;
+ F MemberStruct;
+
+ void testCaptureByValue(int Param, F f) {
+int x = 3;
+int y = 4;
+auto AAA = std::bind(add, x, y);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto AAA = [x, y] { return add(x, y); };
+
+// When the captured variable is repeated, it should only appear in the capture list once.
+auto BBB = std::bind(add, x, x);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto BBB = [x] { return add(x, x); };
+
+int LocalVariable;
+// Global variables shouldn't be captured at all, and members should be captured through this.
+auto CCC = std::bind(add, MemberVariable, GlobalVariable);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto CCC = [this] { return add(MemberVariable, GlobalVariable); };
+
+// Static member variables shouldn't be captured, but locals should
+auto DDD = std::bind(add, TestCaptureByValueStruct::StaticMemberVariable, LocalVariable);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto DDD
[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check
zturner marked 2 inline comments as done.
zturner added inline comments.
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:116
+static const Expr *ignoreTemporariesAndImplicitCasts(const Expr *E) {
+ if (const auto *T = dyn_cast(E))
+return ignoreTemporariesAndImplicitCasts(T->GetTemporaryExpr());
aaron.ballman wrote:
> What about `CXXBindTemporaryExpr`?
>
> Would `Expr::IgnoreImplicit()` do too much stripping because it removes
> `FullExpr` as well?
I don't actually know. This patch is literally my first exposure to clang's
frontend and AST manipulations, so I was kind of just trying different things
until something worked here. I didn't even know about `IgnoreImplicit()` or
`FullExpr`. I'll play around with them and report back.
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:153
+
+ return HNM.matchesNode(*dyn_cast(D));
+}
aaron.ballman wrote:
> This should probably use `cast<>` if it's going to assume the returned value
> is never null.
Good point. Since we're talking about this code anyway, it felt super hacky to
instantiate an AST matcher just to check for the qualified name of a Decl. Is
there a better way to do this?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70368/new/
https://reviews.llvm.org/D70368
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check
aaron.ballman added a comment.
This is a great re-write, thank you for working on it!
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:105
+ CallableInfo Callable;
+
+ SmallVector BindArguments;
You can remove some newlines here.
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:116
+static const Expr *ignoreTemporariesAndImplicitCasts(const Expr *E) {
+ if (const auto *T = dyn_cast(E))
+return ignoreTemporariesAndImplicitCasts(T->GetTemporaryExpr());
What about `CXXBindTemporaryExpr`?
Would `Expr::IgnoreImplicit()` do too much stripping because it removes
`FullExpr` as well?
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:124
+static const Expr *ignoreTemporariesAndPointers(const Expr *E) {
+
+ if (const auto *T = dyn_cast(E))
Spurious newline
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:130
+
+ if (const auto *T = dyn_cast(E))
+return ignoreTemporariesAndPointers(T->GetTemporaryExpr());
Similar question here about bound temporaries as above.
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:143-147
+ std::string S;
+ llvm::raw_string_ostream OS(S);
+ OS << "capture" << CaptureIndex++;
+ OS.flush();
+ return S;
`llvm::utostr()` instead of using a streaming interface?
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:144
+ std::string S;
+ llvm::raw_string_ostream OS(S);
+ OS << "capture" << CaptureIndex++;
I think you can drop the `llvm::` here and elsewhere in the file.
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:153
+
+ return HNM.matchesNode(*dyn_cast(D));
+}
This should probably use `cast<>` if it's going to assume the returned value is
never null.
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:196-199
+ for (const auto *Child : Statement->children()) {
+if (anyDescendantIsLocal(Child))
+ return true;
+ }
`return llvm::any_of(Statement->children(), anyDescendantIsLocal);`?
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:431-434
+ if (!Candidates.empty())
+return Candidates;
+
+ return Candidates;
It seems like `return Candidates;` will suffice. :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70368/new/
https://reviews.llvm.org/D70368
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check
Eugene.Zelenko added a comment. Please describe changes in documentation and Release Notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70368/new/ https://reviews.llvm.org/D70368 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check
zturner created this revision.
zturner added reviewers: aaron.ballman, dblaikie, jbcoe, NoQ.
Herald added a subscriber: xazax.hun.
This represents largely a full re-write of modernize-avoid-bind, adding
significant new functionality in the process. In particular:
1. Both boost::bind and std::bind are now supported
2. Function objects are supported in addition to functions
3. Member functions are supported
4. Nested calls are supported using capture-init syntax
5. `std::ref()` and `boost::ref()` are now recognized, and will capture by
reference.
6. Rather than capturing with a global `=`, we now build up an individual
capture list that is both necessary and sufficient for the call.
7. Fixits are supported in a much larger variety of scenarios than before.
All previous tests pass under the re-write, but a large number of new tests
have been added as well.
I don't know who the best person to review this is, so I've put a couple of
people on here. Feel free to re-direct if there's someone better.
https://reviews.llvm.org/D70368
Files:
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h
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
@@ -8,75 +8,51 @@
template
bind_rt bind(Fp &&, Arguments &&...);
}
+
+template
+T ref(T );
}
-int add(int x, int y) { return x + y; }
+namespace boost {
+template
+class bind_rt {};
-void f() {
- auto clj = std::bind(add, 2, 2);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind]
- // CHECK-FIXES: auto clj = [] { return add(2, 2); };
-}
+template
+bind_rt bind(const Fp &, Arguments...);
+} // namespace boost
-void g() {
- int x = 2;
- int y = 2;
- auto clj = std::bind(add, x, y);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [=] { return add(x, y); };
-}
+namespace C {
+int add(int x, int y) { return x + y; }
+} // namespace C
-struct placeholder {};
-placeholder _1;
-placeholder _2;
+struct Foo {
+ static int add(int x, int y) { return x + y; }
+};
-void h() {
- int x = 2;
- auto clj = std::bind(add, x, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); };
-}
+struct D {
+ D() = default;
+ void operator()(int x, int y) const {}
-struct A;
-struct B;
-bool ABTest(const A &, const B &);
+ void MemberFunction(int x) {}
-void i() {
- auto BATest = std::bind(ABTest, _2, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); };
-}
+ static D *create();
+};
-void j() {
- auto clj = std::bind(add, 2, 2, 2);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for argument mismatches.
- // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2);
-}
+struct F {
+ F(int x) {}
+ ~F() {}
-void k() {
- auto clj = std::bind(add, _1, _1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for reused placeholders.
- // CHECK-FIXES: auto clj = std::bind(add, _1, _1);
-}
+ int get() { return 42; }
+};
-void m() {
- auto clj = std::bind(add, 1, add(2, 5));
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // No fix is applied for nested calls.
- // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5));
-}
+void UseF(F);
-namespace C {
- int add(int x, int y){ return x + y; }
-}
+struct placeholder {};
+placeholder _1;
+placeholder _2;
-void n() {
- auto clj = std::bind(C::add, 1, 1);
- // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
- // CHECK-FIXES: auto clj = [] { return C::add(1, 1); };
-}
+int add(int x, int y) { return x + y; }
+int addThree(int x, int y, int z) { return x + y + z; }
// Let's fake a minimal std::function-like facility.
namespace std {
@@ -114,10 +90,188 @@
void Reset(std::function);
};
-void test(Thing *t) {
+int GlobalVariable = 42;
+
+struct TestCaptureByValueStruct {
+ int MemberVariable;
+ static int StaticMemberVariable;
+ F MemberStruct;
+
+ void testCaptureByValue(int Param, F f) {
+int x = 3;
+int y = 4;
+auto AAA = std::bind(add, x, y);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto AAA = [x, y] { return add(x, y); };
+
+// When the captured variable is repeated, it should only appear in the capture list once.
+auto BBB = std::bind(add, x, x);
+
