Re: [PATCH] D16962: clang-tidy: avoid std::bind
This revision was automatically updated to reflect the committed changes. Closed by commit rL269341: [clang-tidy] Adds modernize-avoid-bind check (authored by jbcoe). Changed prior to commit: http://reviews.llvm.org/D16962?vs=57067&id=57095#toc Repository: rL LLVM http://reviews.llvm.org/D16962 Files: clang-tools-extra/trunk/clang-tidy/modernize/AvoidBindCheck.cpp clang-tools-extra/trunk/clang-tidy/modernize/AvoidBindCheck.h clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-avoid-bind.rst clang-tools-extra/trunk/test/clang-tidy/modernize-avoid-bind.cpp Index: clang-tools-extra/trunk/clang-tidy/modernize/AvoidBindCheck.h === --- clang-tools-extra/trunk/clang-tidy/modernize/AvoidBindCheck.h +++ clang-tools-extra/trunk/clang-tidy/modernize/AvoidBindCheck.h @@ -0,0 +1,36 @@ +//===--- AvoidBindCheck.h - clang-tidy---*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===--===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_AVOID_BIND_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_AVOID_BIND_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace modernize { + +/// Replace simple uses of std::bind with a lambda. +/// +/// FIXME: Add support for function references and member function references. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-avoid-std-bind.html +class AvoidBindCheck : public ClangTidyCheck { +public: + AvoidBindCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; +} // namespace modernize +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_AVOID_BIND_H Index: clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt === --- clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt +++ clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt @@ -1,6 +1,7 @@ set(LLVM_LINK_COMPONENTS support) add_clang_library(clangTidyModernizeModule + AvoidBindCheck.cpp DeprecatedHeadersCheck.cpp LoopConvertCheck.cpp LoopConvertUtils.cpp Index: clang-tools-extra/trunk/clang-tidy/modernize/AvoidBindCheck.cpp === --- clang-tools-extra/trunk/clang-tidy/modernize/AvoidBindCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/modernize/AvoidBindCheck.cpp @@ -0,0 +1,163 @@ +//===--- AvoidBindCheck.cpp - clang-tidy===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===--===// +#include "AvoidBindCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" +#include +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace modernize { + +namespace { +enum BindArgumentKind { BK_Temporary, BK_Placeholder, BK_CallExpr, BK_Other }; + +struct BindArgument { + StringRef Tokens; + BindArgumentKind Kind = BK_Other; + size_t PlaceHolderIndex = 0; +}; + +} // end namespace + +static SmallVector +buildBindArguments(const MatchFinder::MatchResult &Result, const CallExpr *C) { + SmallVector BindArguments; + llvm::Regex MatchPlaceholder("^_([0-9]+)$"); + + // Start at index 1 as first argument to bind is the function name. + for (size_t I = 1, ArgCount = C->getNumArgs(); I < ArgCount; ++I) { +const Expr *E = C->getArg(I); +BindArgument B; +if (const auto *M = dyn_cast(E)) { + const auto *TE = M->GetTemporaryExpr(); + B.Kind = isa(TE) ? BK_CallExpr : BK_Temporary; +} + +B.Tokens = Lexer::getSourceText( +CharSourceRange::getTokenRange(E->getLocStart(), E->getLocEnd()), +*Result.SourceManager, Result.Context->getLangOpts()); + +SmallVector Matches; +if (B.Kind == BK_Other && MatchPlaceholder.match(B.Tokens, &Matches)) { + B.Kind = BK_Placeholder; + B.PlaceHolderIndex = std::stoi(Matches[1]); +} +BindArguments.push_back(B); + } + return BindArguments; +} + +static void addPlaceholderArgs(const ArrayRef Args, +
Re: [PATCH] D16962: clang-tidy: avoid std::bind
jbcoe updated this revision to Diff 57067. jbcoe added a comment. update diff to allow arc to apply patch. http://reviews.llvm.org/D16962 Files: clang-tidy/modernize/AvoidBindCheck.cpp clang-tidy/modernize/AvoidBindCheck.h clang-tidy/modernize/CMakeLists.txt clang-tidy/modernize/ModernizeTidyModule.cpp docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/modernize-avoid-bind.rst test/clang-tidy/modernize-avoid-bind.cpp Index: test/clang-tidy/modernize-avoid-bind.cpp === --- /dev/null +++ test/clang-tidy/modernize-avoid-bind.cpp @@ -0,0 +1,70 @@ +// RUN: %check_clang_tidy %s modernize-avoid-bind %t -- -- -std=c++14 + +namespace std { +inline namespace impl { +template +class bind_rt {}; + +template +bind_rt bind(Fp &&, Arguments &&...); +} +} + +int add(int x, int y) { return x + y; } + +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); }; +} + +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); }; +} + +struct placeholder {}; +placeholder _1; +placeholder _2; + +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 A; +struct B; +bool ABTest(const A &, const B &); + +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); }; +} + +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 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); +} + +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)); +} + Index: docs/clang-tidy/checks/modernize-avoid-bind.rst === --- /dev/null +++ docs/clang-tidy/checks/modernize-avoid-bind.rst @@ -0,0 +1,38 @@ +.. title:: clang-tidy - modernize-avoid-std-bind + +modernize-avoid-bind +== + +The check finds uses of ``std::bind`` and replaces simple uses with lambdas. +Lambdas will use value-capture where required. + +Right now it only handles free functions, not member functions. + +Given: + +.. code:: C++ + + int add(int x, int y) { return x + y; } + +Then: + +.. code:: C++ + + void f() { +int x = 2; +auto clj = std::bind(add, x, _1); + } + +is replaced by: + +.. code:: C++ + + void f() { +int x = 2; +auto clj = [=](auto && arg1) { return add(x, arg1); }; + } + +``std::bind`` can be hard to read and can result in larger object files and +binaries due to type information that will not be produced by equivalent +lambdas. + Index: docs/clang-tidy/checks/list.rst === --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -91,6 +91,7 @@ misc-unused-raii misc-unused-using-decls misc-virtual-near-miss + modernize-avoid-bind modernize-deprecated-headers modernize-loop-convert modernize-make-shared Index: clang-tidy/modernize/ModernizeTidyModule.cpp === --- clang-tidy/modernize/ModernizeTidyModule.cpp +++ clang-tidy/modernize/ModernizeTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "AvoidBindCheck.h" #include "DeprecatedHeadersCheck.h" #include "LoopConvertCheck.h" #include "MakeSharedCheck.h" @@ -34,6 +35,8 @@ class ModernizeModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { +CheckFactories.registerCheck( +"modernize-avoid-bind"); CheckFactories.registerCheck( "modernize-deprecated-headers"); CheckFactories.registerCheck("modernize-loop-convert"); Index: clang-tidy/modernize/CMakeLists.txt === --- clang-tidy/modernize/CMakeLists.txt +++ clang-tidy/modernize/CMakeLists.txt @@ -1,6 +1,7 @@ s
Re: [PATCH] D16962: clang-tidy: avoid std::bind
jbcoe added a comment. I can submit the patch myself, thanks. Thanks again for taking the time to give such a thorough review. http://reviews.llvm.org/D16962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16962: clang-tidy: avoid std::bind
alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. Looks good. Thank you! Do you need someone to submit the patch for you? Comment at: clang-tidy/modernize/AvoidBindCheck.cpp:28 @@ +27,3 @@ + BindArgumentKind Kind = BK_Other; + size_t PlaceHolderIndex = 0; +}; Answering my own question: according to https://msdn.microsoft.com/en-us/library/hh567368.aspx non-static member initializers are supported by VS2013. This should be fine. http://reviews.llvm.org/D16962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16962: clang-tidy: avoid std::bind
jbcoe updated this revision to Diff 57022. jbcoe marked 6 inline comments as done. jbcoe added a comment. Apply fixes from review. http://reviews.llvm.org/D16962 Files: clang-tidy/modernize/AvoidBindCheck.cpp clang-tidy/modernize/AvoidBindCheck.h clang-tidy/modernize/CMakeLists.txt clang-tidy/modernize/ModernizeTidyModule.cpp docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/modernize-avoid-bind.rst test/clang-tidy/modernize-avoid-bind.cpp Index: test/clang-tidy/modernize-avoid-bind.cpp === --- /dev/null +++ test/clang-tidy/modernize-avoid-bind.cpp @@ -0,0 +1,70 @@ +// RUN: %check_clang_tidy %s modernize-avoid-bind %t -- -- -std=c++14 + +namespace std { +inline namespace impl { +template +class bind_rt {}; + +template +bind_rt bind(Fp &&, Arguments &&...); +} +} + +int add(int x, int y) { return x + y; } + +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); }; +} + +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); }; +} + +struct placeholder {}; +placeholder _1; +placeholder _2; + +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 A; +struct B; +bool ABTest(const A &, const B &); + +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); }; +} + +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 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); +} + +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)); +} + Index: docs/clang-tidy/checks/modernize-avoid-bind.rst === --- /dev/null +++ docs/clang-tidy/checks/modernize-avoid-bind.rst @@ -0,0 +1,38 @@ +.. title:: clang-tidy - modernize-avoid-std-bind + +modernize-avoid-bind +== + +The check finds uses of ``std::bind`` and replaces simple uses with lambdas. +Lambdas will use value-capture where required. + +Right now it only handles free functions, not member functions. + +Given: + +.. code:: C++ + + int add(int x, int y) { return x + y; } + +Then: + +.. code:: C++ + + void f() { +int x = 2; +auto clj = std::bind(add, x, _1); + } + +is replaced by: + +.. code:: C++ + + void f() { +int x = 2; +auto clj = [=](auto && arg1) { return add(x, arg1); }; + } + +``std::bind`` can be hard to read and can result in larger object files and +binaries due to type information that will not be produced by equivalent +lambdas. + Index: docs/clang-tidy/checks/list.rst === --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -87,6 +87,7 @@ misc-unused-raii misc-unused-using-decls misc-virtual-near-miss + modernize-avoid-bind modernize-deprecated-headers modernize-loop-convert modernize-make-unique Index: clang-tidy/modernize/ModernizeTidyModule.cpp === --- clang-tidy/modernize/ModernizeTidyModule.cpp +++ clang-tidy/modernize/ModernizeTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "AvoidBindCheck.h" #include "DeprecatedHeadersCheck.h" #include "LoopConvertCheck.h" #include "MakeUniqueCheck.h" @@ -32,6 +33,8 @@ class ModernizeModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { +CheckFactories.registerCheck( +"modernize-avoid-bind"); CheckFactories.registerCheck( "modernize-deprecated-headers"); CheckFactories.registerCheck("modernize-loop-convert"); Index: clang-tidy/modernize/CMakeLists.txt === --- clang-tidy/modernize/CMakeLists.txt +++ clang-tidy/modernize/CMakeList
Re: [PATCH] D16962: clang-tidy: avoid std::bind
alexfh requested changes to this revision. This revision now requires changes to proceed. Comment at: clang-tidy/modernize/AvoidBindCheck.cpp:27 @@ +26,3 @@ + StringRef Tokens; + BindArgumentKind Kind = BK_Other; + size_t PlaceHolderIndex = 0; I wonder whether VS2013 supports inline member initializers? Comment at: clang-tidy/modernize/AvoidBindCheck.cpp:77 @@ +76,3 @@ + for (size_t I = 1; I <= PlaceholderCount; ++I) { +Stream << Delimiter << "auto &&" + << " arg" << I; clang-format, please. Also, please join the two string literals here. Comment at: clang-tidy/modernize/AvoidBindCheck.cpp:116 @@ +115,3 @@ +void AvoidBindCheck::check(const MatchFinder::MatchResult &Result) { + if (!getLangOpts().CPlusPlus14) // Need C++14 for generic lambdas. +return; This check should be done in `registerMatchers` to avoid unnecessary work. Comment at: clang-tidy/modernize/AvoidBindCheck.cpp:161 @@ +160,3 @@ + MatchedDecl->getLocStart(), + Lexer::getLocForEndOfToken(MatchedDecl->getLocEnd(), 0, + *Result.SourceManager, I don't think you need `getLocForEndOfToken`, since `FixItHint::CreateReplacement(SourceRange)` treats the range as a token range. Comment at: docs/clang-tidy/checks/modernize-avoid-bind.rst:26 @@ +25,3 @@ + +.. code:: C++ + void f() { `..code:: C++` directive should be followed by an empty line. Please check the documentation builds without warnings. Comment at: docs/clang-tidy/checks/modernize-avoid-bind.rst:32 @@ +31,3 @@ + +We created this check because ``std::bind`` can be hard to read and can result +in larger object files and binaries due to type information that will not be s/We created this check because // Comment at: docs/clang-tidy/checks/readability-avoid-std-bind.rst:13 @@ +12,3 @@ + +.. code:: C++ + int add(int x, int y) { return x + y; } alexfh wrote: > `.. code::` should be followed by an empty line. > > Please also verify the documentation can be built without errors. On Ubuntu > this boils down to: > > 1. Install sphinx: > > $ sudo apt-get install sphinx-common > > 2. Enable `LLVM_BUILD_DOCS` and maybe some other options in cmake. > > 3. `ninja docs-clang-tools-html` (or something similar, if you use make). This comment is not addressed. Comment at: test/clang-tidy/modernize-avoid-bind.cpp:25 @@ +24,3 @@ + auto clj = std::bind(add, x, y); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // CHECK-FIXES: auto clj = [=] { return add(x, y); }; Remove `[modernize-avoid-bind]` from all CHECK-MESSAGES lines except for the first one. http://reviews.llvm.org/D16962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16962: clang-tidy: avoid std::bind
Modernize it is then. The check currently only catches std::bind but further work can address that. Jon > On 4 May 2016, at 22:40, Arthur O'Dwyer wrote: > >> On Wed, May 4, 2016 at 1:43 PM, Aaron Ballman via cfe-commits >> wrote: >> jbcoe wrote: >> > aaron.ballman wrote: >> > > I believe we use "modernize" to really mean "migrate from the old way to >> > > the new way", which this definitely fits into since I think the point to >> > > this check is to replace bind with better alternatives. >> > Would you prefer it to be in `modernize`? I can be easily convinced either >> > way and am happy to move it. If I do move it I might add a script to >> > facilitate doing so. >> My preference is for modernize, your preference is for readability, so I >> say: make @alexfh the tie-breaker! ;-) Alex, what are your thoughts? This >> seems like a heuristic we may want to state in our documentation to help >> others decide where to put new checks in the future as well. > > FWIW, I'd prefer "modernize", and I'll point out that these waters are > muddied by the fact that three of the old ways (boost::bind, std::bind1st, > std::bind2nd) all existed prior to C++11, so the fact that one of the old > ways (std::bind) was introduced in C++11 doesn't matter so much. > (I haven't looked, but I'd assume that this clang-tidy check catches all four > cases, right?) > > –Arthur ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16962: clang-tidy: avoid std::bind
jbcoe removed a reviewer: djasper. jbcoe updated this revision to Diff 56273. jbcoe added a comment. Move to modernize. Rename to `avoid-bind` as a later patch will add support for `boost::bind`, `std::bind1st` etc. http://reviews.llvm.org/D16962 Files: clang-tidy/modernize/AvoidBindCheck.cpp clang-tidy/modernize/AvoidBindCheck.h clang-tidy/modernize/CMakeLists.txt clang-tidy/modernize/ModernizeTidyModule.cpp docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/modernize-avoid-bind.rst test/clang-tidy/modernize-avoid-bind.cpp Index: test/clang-tidy/modernize-avoid-bind.cpp === --- /dev/null +++ test/clang-tidy/modernize-avoid-bind.cpp @@ -0,0 +1,70 @@ +// RUN: %check_clang_tidy %s modernize-avoid-bind %t -- -- -std=c++14 + +namespace std { +inline namespace impl { +template +class bind_rt {}; + +template +bind_rt bind(Fp &&, Arguments &&...); +} +} + +int add(int x, int y) { return x + y; } + +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); }; +} + +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 [modernize-avoid-bind] + // CHECK-FIXES: auto clj = [=] { return add(x, y); }; +} + +struct placeholder {}; +placeholder _1; +placeholder _2; + +void h() { + int x = 2; + auto clj = std::bind(add, x, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); }; +} + +struct A; +struct B; +bool ABTest(const A &, const B &); + +void i() { + auto BATest = std::bind(ABTest, _2, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); }; +} + +void j() { + auto clj = std::bind(add, 2, 2, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // No fix is applied for argument mismatches. + // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2); +} + +void k() { + auto clj = std::bind(add, _1, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // No fix is applied for reused placeholders. + // CHECK-FIXES: auto clj = std::bind(add, _1, _1); +} + +void m() { + auto clj = std::bind(add, 1, add(2, 5)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // No fix is applied for nested calls. + // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5)); +} + Index: docs/clang-tidy/checks/modernize-avoid-bind.rst === --- /dev/null +++ docs/clang-tidy/checks/modernize-avoid-bind.rst @@ -0,0 +1,35 @@ +.. title:: clang-tidy - modernize-avoid-std-bind + +modernize-avoid-bind +== + +The check finds uses of ``std::bind`` and replaces simple uses with lambdas. +Lambdas will use value-capture where required. + +Right now it only handles free functions, not member functions. + +Given: + +.. code:: C++ + int add(int x, int y) { return x + y; } + +Then: + +.. code:: C++ + void f() { +int x = 2; +auto clj = std::bind(add, x, _1); + } + +is replaced by: + +.. code:: C++ + void f() { +int x = 2; +auto clj = [=](auto && arg1) { return add(x, arg1); }; + } + +We created this check because ``std::bind`` can be hard to read and can result +in larger object files and binaries due to type information that will not be +produced by equivalent lambdas. + Index: docs/clang-tidy/checks/list.rst === --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -87,6 +87,7 @@ misc-unused-raii misc-unused-using-decls misc-virtual-near-miss + modernize-avoid-bind modernize-deprecated-headers modernize-loop-convert modernize-make-unique Index: clang-tidy/modernize/ModernizeTidyModule.cpp === --- clang-tidy/modernize/ModernizeTidyModule.cpp +++ clang-tidy/modernize/ModernizeTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "AvoidBindCheck.h" #include "DeprecatedHeadersCheck.h" #include "LoopConvertCheck.h" #include "MakeUniqueCheck.h" @@ -32,6 +33,8 @@ class ModernizeModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { +CheckFactories.registerCheck( +"modernize-avoid-bind"); CheckFactories.registerCheck( "modernize-deprecated-headers"); Che
Re: [PATCH] D16962: clang-tidy: avoid std::bind
On Wed, May 4, 2016 at 1:43 PM, Aaron Ballman via cfe-commits < cfe-commits@lists.llvm.org> wrote: > jbcoe wrote: > > aaron.ballman wrote: > > > I believe we use "modernize" to really mean "migrate from the old way > to the new way", which this definitely fits into since I think the point to > this check is to replace bind with better alternatives. > > Would you prefer it to be in `modernize`? I can be easily convinced > either way and am happy to move it. If I do move it I might add a script to > facilitate doing so. > My preference is for modernize, your preference is for readability, so I > say: make @alexfh the tie-breaker! ;-) Alex, what are your thoughts? This > seems like a heuristic we may want to state in our documentation to help > others decide where to put new checks in the future as well. > FWIW, I'd prefer "modernize", and I'll point out that these waters are muddied by the fact that three of the old ways (boost::bind, std::bind1st, std::bind2nd) all existed prior to C++11, so the fact that *one* of the old ways (std::bind) was introduced in C++11 doesn't matter so much. (I haven't looked, but I'd assume that this clang-tidy check catches all four cases, right?) –Arthur ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16962: clang-tidy: avoid std::bind
aaron.ballman added inline comments. Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:17 @@ -15,2 +16,2 @@ #include "ContainerSizeEmptyCheck.h" #include "DeletedDefaultCheck.h" jbcoe wrote: > aaron.ballman wrote: > > I believe we use "modernize" to really mean "migrate from the old way to > > the new way", which this definitely fits into since I think the point to > > this check is to replace bind with better alternatives. > Would you prefer it to be in `modernize`? I can be easily convinced either > way and am happy to move it. If I do move it I might add a script to > facilitate doing so. My preference is for modernize, your preference is for readability, so I say: make @alexfh the tie-breaker! ;-) Alex, what are your thoughts? This seems like a heuristic we may want to state in our documentation to help others decide where to put new checks in the future as well. http://reviews.llvm.org/D16962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16962: clang-tidy: avoid std::bind
jbcoe added inline comments. Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:17 @@ -15,2 +16,2 @@ #include "ContainerSizeEmptyCheck.h" #include "DeletedDefaultCheck.h" aaron.ballman wrote: > I believe we use "modernize" to really mean "migrate from the old way to the > new way", which this definitely fits into since I think the point to this > check is to replace bind with better alternatives. Would you prefer it to be in `modernize`? I can be easily convinced either way and am happy to move it. If I do move it I might add a script to facilitate doing so. http://reviews.llvm.org/D16962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16962: clang-tidy: avoid std::bind
aaron.ballman added inline comments. Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:17 @@ -15,2 +16,2 @@ #include "ContainerSizeEmptyCheck.h" #include "DeletedDefaultCheck.h" I believe we use "modernize" to really mean "migrate from the old way to the new way", which this definitely fits into since I think the point to this check is to replace bind with better alternatives. http://reviews.llvm.org/D16962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16962: clang-tidy: avoid std::bind
alexfh added a comment. Please regenerate the patch with the full context (see http://llvm.org/docs/Phabricator.html). Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:77 @@ +76,3 @@ + for (size_t I = 1; I <= PlaceholderCount; ++I) { +Stream << Delimiter << "auto &&" + << " arg" << I; clang-format, please. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:116 @@ +115,3 @@ +void AvoidStdBindCheck::check(const MatchFinder::MatchResult &Result) { + if (!getLangOpts().CPlusPlus14) // Need C++14 for generic lambdas. +return; This should be done in `registerMatchers` instead. Comment at: docs/clang-tidy/checks/readability-avoid-std-bind.rst:13 @@ +12,3 @@ + +.. code:: C++ + int add(int x, int y) { return x + y; } `.. code::` should be followed by an empty line. Please also verify the documentation can be built without errors. On Ubuntu this boils down to: 1. Install sphinx: $ sudo apt-get install sphinx-common 2. Enable `LLVM_BUILD_DOCS` and maybe some other options in cmake. 3. `ninja docs-clang-tools-html` (or something similar, if you use make). http://reviews.llvm.org/D16962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16962: clang-tidy: avoid std::bind
jbcoe updated this revision to Diff 55974. jbcoe added a comment. Remove unused function `isStdBind`. http://reviews.llvm.org/D16962 Files: clang-tidy/readability/AvoidStdBindCheck.cpp clang-tidy/readability/AvoidStdBindCheck.h clang-tidy/readability/CMakeLists.txt clang-tidy/readability/ReadabilityTidyModule.cpp docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/readability-avoid-std-bind.rst test/clang-tidy/readability-avoid-std-bind.cpp Index: test/clang-tidy/readability-avoid-std-bind.cpp === --- /dev/null +++ test/clang-tidy/readability-avoid-std-bind.cpp @@ -0,0 +1,70 @@ +// RUN: %check_clang_tidy %s readability-avoid-std-bind %t -- -- -std=c++14 + +namespace std { +inline namespace impl { +template +class bind_rt {}; + +template +bind_rt bind(Fp &&, Arguments &&...); +} +} + +int add(int x, int y) { return x + y; } + +void f() { + auto clj = std::bind(add, 2, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] + // CHECK-FIXES: auto clj = [] { return add(2, 2); }; +} + +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 [readability-avoid-std-bind] + // CHECK-FIXES: auto clj = [=] { return add(x, y); }; +} + +struct placeholder {}; +placeholder _1; +placeholder _2; + +void h() { + int x = 2; + auto clj = std::bind(add, x, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] + // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); }; +} + +struct A; +struct B; +bool ABTest(const A &, const B &); + +void i() { + auto BATest = std::bind(ABTest, _2, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind [readability-avoid-std-bind] + // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); }; +} + +void j() { + auto clj = std::bind(add, 2, 2, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] + // No fix is applied for argument mismatches. + // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2); +} + +void k() { + auto clj = std::bind(add, _1, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] + // No fix is applied for reused placeholders. + // CHECK-FIXES: auto clj = std::bind(add, _1, _1); +} + +void m() { + auto clj = std::bind(add, 1, add(2, 5)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] + // No fix is applied for nested calls. + // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5)); +} + Index: docs/clang-tidy/checks/readability-avoid-std-bind.rst === --- /dev/null +++ docs/clang-tidy/checks/readability-avoid-std-bind.rst @@ -0,0 +1,35 @@ +.. title:: clang-tidy - readability-avoid-std-bind + +readability-avoid-std-bind +== + +The check finds uses of ``std::bind`` and replaces simple uses with lambdas. +Lambdas will use value-capture where required. + +Right now it only handles free functions, not member functions. + +Given: + +.. code:: C++ + int add(int x, int y) { return x + y; } + +Then: + +.. code:: C++ + void f() { +int x = 2; +auto clj = std::bind(add, x, _1); + } + +is replaced by: + +.. code:: C++ + void f() { +int x = 2; +auto clj = [=](auto && arg1) { return add(x, arg1); }; + } + +We created this check because ``std::bind`` can be hard to read and can result +in larger object files and binaries due to type information that will not be +produced by equivalent lambdas. + Index: docs/clang-tidy/checks/list.rst === --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -105,6 +105,7 @@ performance-unnecessary-copy-initialization performance-unnecessary-value-param readability-avoid-const-params-in-decls + readability-avoid-std-bind readability-braces-around-statements readability-container-size-empty readability-deleted-default Index: clang-tidy/readability/ReadabilityTidyModule.cpp === --- clang-tidy/readability/ReadabilityTidyModule.cpp +++ clang-tidy/readability/ReadabilityTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" #include "AvoidConstParamsInDecls.h" +#include "AvoidStdBindCheck.h" #include "BracesAroundStatementsCheck.h" #include "ContainerSizeEmptyCheck.h" #include "DeletedDefaultCheck.h" @@ -37,6 +38,8 @@ void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck( "readability-avoid-const-params-in-decls"); +
Re: [PATCH] D16962: clang-tidy: avoid std::bind
jbcoe updated this revision to Diff 55970. jbcoe marked 16 inline comments as done. jbcoe added a comment. Minor fixes from review. http://reviews.llvm.org/D16962 Files: clang-tidy/readability/AvoidStdBindCheck.cpp clang-tidy/readability/AvoidStdBindCheck.h clang-tidy/readability/CMakeLists.txt clang-tidy/readability/ReadabilityTidyModule.cpp docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/readability-avoid-std-bind.rst test/clang-tidy/readability-avoid-std-bind.cpp Index: test/clang-tidy/readability-avoid-std-bind.cpp === --- /dev/null +++ test/clang-tidy/readability-avoid-std-bind.cpp @@ -0,0 +1,70 @@ +// RUN: %check_clang_tidy %s readability-avoid-std-bind %t -- -- -std=c++14 + +namespace std { +inline namespace impl { +template +class bind_rt {}; + +template +bind_rt bind(Fp &&, Arguments &&...); +} +} + +int add(int x, int y) { return x + y; } + +void f() { + auto clj = std::bind(add, 2, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] + // CHECK-FIXES: auto clj = [] { return add(2, 2); }; +} + +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 [readability-avoid-std-bind] + // CHECK-FIXES: auto clj = [=] { return add(x, y); }; +} + +struct placeholder {}; +placeholder _1; +placeholder _2; + +void h() { + int x = 2; + auto clj = std::bind(add, x, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] + // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); }; +} + +struct A; +struct B; +bool ABTest(const A &, const B &); + +void i() { + auto BATest = std::bind(ABTest, _2, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind [readability-avoid-std-bind] + // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); }; +} + +void j() { + auto clj = std::bind(add, 2, 2, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] + // No fix is applied for argument mismatches. + // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2); +} + +void k() { + auto clj = std::bind(add, _1, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] + // No fix is applied for reused placeholders. + // CHECK-FIXES: auto clj = std::bind(add, _1, _1); +} + +void m() { + auto clj = std::bind(add, 1, add(2, 5)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] + // No fix is applied for nested calls. + // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5)); +} + Index: docs/clang-tidy/checks/readability-avoid-std-bind.rst === --- /dev/null +++ docs/clang-tidy/checks/readability-avoid-std-bind.rst @@ -0,0 +1,35 @@ +.. title:: clang-tidy - readability-avoid-std-bind + +readability-avoid-std-bind +== + +The check finds uses of ``std::bind`` and replaces simple uses with lambdas. +Lambdas will use value-capture where required. + +Right now it only handles free functions, not member functions. + +Given: + +.. code:: C++ + int add(int x, int y) { return x + y; } + +Then: + +.. code:: C++ + void f() { +int x = 2; +auto clj = std::bind(add, x, _1); + } + +is replaced by: + +.. code:: C++ + void f() { +int x = 2; +auto clj = [=](auto && arg1) { return add(x, arg1); }; + } + +We created this check because ``std::bind`` can be hard to read and can result +in larger object files and binaries due to type information that will not be +produced by equivalent lambdas. + Index: docs/clang-tidy/checks/list.rst === --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -105,6 +105,7 @@ performance-unnecessary-copy-initialization performance-unnecessary-value-param readability-avoid-const-params-in-decls + readability-avoid-std-bind readability-braces-around-statements readability-container-size-empty readability-deleted-default Index: clang-tidy/readability/ReadabilityTidyModule.cpp === --- clang-tidy/readability/ReadabilityTidyModule.cpp +++ clang-tidy/readability/ReadabilityTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" #include "AvoidConstParamsInDecls.h" +#include "AvoidStdBindCheck.h" #include "BracesAroundStatementsCheck.h" #include "ContainerSizeEmptyCheck.h" #include "DeletedDefaultCheck.h" @@ -37,6 +38,8 @@ void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck( "readability-avo
Re: [PATCH] D16962: clang-tidy: avoid std::bind
jbcoe added inline comments. Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:37 @@ -35,1 +36,3 @@ +CheckFactories.registerCheck( +"readability-avoid-std-bind"); CheckFactories.registerCheck( aaron.ballman wrote: > I kind of wonder if this should be in modernize instead of readability? Tough > call, and I'm fine with either place, just wanted to pose the question. Given that lambdas and `std::bind` both arrived in C++11, `modernize` seems a misnomer. I think `readability` is better fit. http://reviews.llvm.org/D16962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16962: clang-tidy: avoid std::bind
aaron.ballman added inline comments. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:116 @@ +115,3 @@ + Finder->addMatcher( + callExpr(callee(namedDecl(isStdBind())), + hasArgument(0, declRefExpr(to(functionDecl().bind("f") alexfh wrote: > You should be able to use `hasName("std::bind")` instead. I think that has to be `hasName("::std::bind")` to support `namespace mine { namespace std { void bind(); } }` Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:126 @@ +125,3 @@ + + if (!getLangOpts().CPlusPlus14) // Need C++14 for generic lambdas. +return; Might as well move this to the top of check() since there's no need to create the diagnostic or find the matched decl unless this check passes. Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:37 @@ -35,1 +36,3 @@ +CheckFactories.registerCheck( +"readability-avoid-std-bind"); CheckFactories.registerCheck( I kind of wonder if this should be in modernize instead of readability? Tough call, and I'm fine with either place, just wanted to pose the question. Comment at: docs/clang-tidy/checks/readability-avoid-std-bind.rst:9 @@ +8,3 @@ + +Right now it only handles free functions not member functions. + alexfh wrote: > Add a comma before "not"? or "and not" instead of a comma, if verbosity is your thing. Comment at: test/clang-tidy/readability-avoid-std-bind.cpp:1 @@ +1,2 @@ +// RUN: %check_clang_tidy %s readability-avoid-std-bind %t -- -- -std=c++14 + Can you clang-format this file as well? Comment at: test/clang-tidy/readability-avoid-std-bind.cpp:21 @@ +20,3 @@ + +// CHECK-FIXES: auto clj = [] { return add(2, 2); }; + This should immediately follow the CHECK-MESSAGES in the same compound statement (here and elsewhere). http://reviews.llvm.org/D16962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16962: clang-tidy: avoid std::bind
alexfh added inline comments. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:46 @@ +45,3 @@ +BindArgument B; +if (const auto * M = dyn_cast(E)) { + const auto * TE = M->GetTemporaryExpr(); In LLVM style this should be `const auto *M`. Please clang-format the file. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:48 @@ +47,3 @@ + const auto * TE = M->GetTemporaryExpr(); + if (dyn_cast(TE)) +B.Kind = BK_CallExpr; Please use `isa` instead of `dyn_cast`, when you don't need the pointer returned by `dyn_cast`. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:49 @@ +48,3 @@ + if (dyn_cast(TE)) +B.Kind = BK_CallExpr; + else I'd prefer to use the conditional operator here: `B.Kind = isa(TE) ? BK_CallExpr : BK_Temporary;`. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:116 @@ +115,3 @@ + Finder->addMatcher( + callExpr(callee(namedDecl(isStdBind())), + hasArgument(0, declRefExpr(to(functionDecl().bind("f") You should be able to use `hasName("std::bind")` instead. Comment at: docs/clang-tidy/checks/readability-avoid-std-bind.rst:6 @@ +5,3 @@ + +Find uses of ``std::bind``. Replace simple uses of ``std::bind`` with lambdas. +Lambdas will use value-capture where required. "The check finds uses of ..." Comment at: docs/clang-tidy/checks/readability-avoid-std-bind.rst:9 @@ +8,3 @@ + +Right now it only handles free functions not member functions. + Add a comma before "not"? Comment at: docs/clang-tidy/checks/readability-avoid-std-bind.rst:11 @@ +10,3 @@ + +Fixits are only generated for simple uses of ``std::bind``. + Please add an example or two of what code is flagged and what suggested fixes are. http://reviews.llvm.org/D16962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16962: clang-tidy: avoid std::bind
jbcoe added inline comments. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:42 @@ +41,3 @@ + + for (size_t I = 1, ArgCount = C->getNumArgs(); I < ArgCount; ++I) { +const Expr *E = C->getArg(I); alexfh wrote: > Please use a range-based for loop over `C->arguments()`. I'm starting at 1 not zero, hence the explicit loop. Elements are non-contiguous so I can't use a restricted ArrayView. I've added a comment saying why it starts at 1, not 0. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:127 @@ +126,3 @@ + auto DiagnosticBuilder = + diag(MatchedDecl->getLocStart(), "avoid using std::bind"); + alexfh wrote: > alexfh wrote: > > Should the message recommend something instead? > In pre-C++11 code the check will just warn without suggesting any > alternative. That will lead to a lot of user confusion. We either need to > restrict the warning to C++14 code or suggest a better alternative even in > pre-C++14 code. The message now recommends using a lambda so I think this is addressed. http://reviews.llvm.org/D16962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16962: clang-tidy: avoid std::bind
jbcoe updated this revision to Diff 50623. jbcoe marked 15 inline comments as done. jbcoe added a comment. Apply requested fixes from review. Thanks for taking time to review this, the patch is much improved for the attention. http://reviews.llvm.org/D16962 Files: clang-tidy/readability/AvoidStdBindCheck.cpp clang-tidy/readability/AvoidStdBindCheck.h clang-tidy/readability/CMakeLists.txt clang-tidy/readability/ReadabilityTidyModule.cpp docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/readability-avoid-std-bind.rst test/clang-tidy/readability-avoid-std-bind.cpp Index: test/clang-tidy/readability-avoid-std-bind.cpp === --- /dev/null +++ test/clang-tidy/readability-avoid-std-bind.cpp @@ -0,0 +1,81 @@ +// RUN: %check_clang_tidy %s readability-avoid-std-bind %t -- -- -std=c++14 + +namespace std { +inline namespace impl { +template +class bind_rt {}; + +template +bind_rt bind(Fp&&, Arguments&& ...); +} +} + +int add(int x, int y) { return x + y; } + +void f() +{ + auto clj = std::bind(add,2,2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] +} + +// CHECK-FIXES: auto clj = [] { return add(2, 2); }; + +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 [readability-avoid-std-bind] +} + +// CHECK-FIXES: auto clj = [=] { return add(x, y); }; + +struct placeholder {}; +placeholder _1; +placeholder _2; + +void h() +{ + int x = 2; + auto clj = std::bind(add,x,_1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] +} + +// CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); }; + +struct A; +struct B; +bool ABTest(const A&, const B&); + +void i() +{ + auto BATest = std::bind(ABTest, _2, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind [readability-avoid-std-bind] +} + +// CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); }; + +void j() +{ + auto clj = std::bind(add, 2, 2, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] +} +// No fix is applied for argument mismatches. +// CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2); + +void k() +{ + auto clj = std::bind(add, _1, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] +} +// No fix is applied for reused placeholders. +// CHECK-FIXES: auto clj = std::bind(add, _1, _1); + +void m() +{ + auto clj = std::bind(add, 1, add(2, 5)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [readability-avoid-std-bind] +} +// No fix is applied for nested calls. +// CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5)); + Index: docs/clang-tidy/checks/readability-avoid-std-bind.rst === --- /dev/null +++ docs/clang-tidy/checks/readability-avoid-std-bind.rst @@ -0,0 +1,16 @@ +.. title:: clang-tidy - readability-avoid-std-bind + +readability-avoid-std-bind +== + +Find uses of ``std::bind``. Replace simple uses of ``std::bind`` with lambdas. +Lambdas will use value-capture where required. + +Right now it only handles free functions not member functions. + +Fixits are only generated for simple uses of ``std::bind``. + +``std::bind`` can be hard to read and can result in larger object files and +binaries due to type information that will not be produced by equivalent +lambdas. + Index: docs/clang-tidy/checks/list.rst === --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -88,6 +88,7 @@ performance-faster-string-find performance-for-range-copy performance-implicit-cast-in-loop + readability-avoid-std-bind readability-braces-around-statements readability-container-size-empty readability-else-after-return Index: clang-tidy/readability/ReadabilityTidyModule.cpp === --- clang-tidy/readability/ReadabilityTidyModule.cpp +++ clang-tidy/readability/ReadabilityTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "AvoidStdBindCheck.h" #include "BracesAroundStatementsCheck.h" #include "ContainerSizeEmptyCheck.h" #include "ElseAfterReturnCheck.h" @@ -32,6 +33,8 @@ class ReadabilityModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { +CheckFactories.registerCheck( +"readability-avoid-std-bind"); CheckFactories.registerCheck( "readability-braces-around-statements"); CheckFactories.registerCheck( Index: clang-tidy/readabilit
Re: [PATCH] D16962: clang-tidy: avoid std::bind
alexfh added inline comments. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:37 @@ +36,3 @@ + +std::vector +buildBindArguments(const MatchFinder::MatchResult &Result, const CallExpr *C) { `SmallVector<>` would be better here, since the number of arguments is usually rather low. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:38 @@ +37,3 @@ +std::vector +buildBindArguments(const MatchFinder::MatchResult &Result, const CallExpr *C) { + std::vector BindArguments; Please make the functions static. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:42 @@ +41,3 @@ + + for (size_t I = 1, ArgCount = C->getNumArgs(); I < ArgCount; ++I) { +const Expr *E = C->getArg(I); Please use a range-based for loop over `C->arguments()`. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:45 @@ +44,3 @@ +BindArgument B; +if (auto M = dyn_cast(E)) { + auto TE = M->GetTemporaryExpr(); Should be `const auto *M` to make it clear M is a pointer. Same for other code like this. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:67 @@ +66,3 @@ + +void addPlaceholderArgs(const std::vector &Args, +llvm::raw_ostream &Stream) { Please use `ArrayRef<>` to pass a constant reference to a sequence of elements. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:100 @@ +99,3 @@ + } + Stream << "); };"; +} I don't like the asymmetry here: the opening parenthesis is added in the caller and the closing parenthesis is added here. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:104 @@ +103,3 @@ +bool isPlaceHolderIndexRepeated(const std::vector &Args) { + std::unordered_map PlaceHolderIndexCounts; + for (const BindArgument &B : Args) { llvm::SmallPtrSet PlaceHolderIndices; for (const BindArgument &B : Args) { if (B.PlaceHolderIndex) { if (!PlaceHolderIndices.insert(B.PlaceHolderIndex).second) return true; } } return false; Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:126 @@ +125,3 @@ + const auto *MatchedDecl = Result.Nodes.getNodeAs("bind"); + auto DiagnosticBuilder = + diag(MatchedDecl->getLocStart(), "avoid using std::bind"); Many other checks use just `Diag`. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:127 @@ +126,3 @@ + auto DiagnosticBuilder = + diag(MatchedDecl->getLocStart(), "avoid using std::bind"); + Should the message recommend something instead? Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:127 @@ +126,3 @@ + auto DiagnosticBuilder = + diag(MatchedDecl->getLocStart(), "avoid using std::bind"); + alexfh wrote: > Should the message recommend something instead? In pre-C++11 code the check will just warn without suggesting any alternative. That will lead to a lot of user confusion. We either need to restrict the warning to C++14 code or suggest a better alternative even in pre-C++14 code. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:129 @@ +128,3 @@ + + if (!getLangOpts().CPlusPlus14) // Need C++14 for generic lambdas +return; nit: Trailing period. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:139 @@ +138,3 @@ + // sharing between outer and inner std:bind invocations. + if (std::find_if(Args.begin(), Args.end(), [](const BindArgument &B) { +return B.Kind == BK_CallExpr; This would be shorter with `std::any_of` or even better with `llvm::any_of` that accepts a range instead of begin/end. Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:163 @@ +162,3 @@ + bool HasCapturedArgument = + std::find_if(Args.begin(), Args.end(), [](const BindArgument &B) { +return B.Kind == BK_Other; Use `llvm::any_of`. Comment at: clang-tidy/readability/AvoidStdBindCheck.h:1 @@ +1,2 @@ +//===--- AvoidStdBindCheck.h - clang-tidy-*- C++ +//-*-===// Join with the next line and remove extra dashes, please. Comment at: docs/clang-tidy/checks/readability-avoid-std-bind.rst:4 @@ +3,3 @@ +readability-avoid-std-bind +=== + nit: Add moar ekwality. Comment at: docs/clang-tidy/checks/readability-avoid-std-bind.rst:6 @@ +5,3 @@ + +Find uses of std::bind. Replace simple uses of std::bind with lambdas. Lambdas +will use value-capture where required. Use double backquotes to highlight inline code snippets like `std::bind`. http://reviews.llvm.org/D16962 ___ cfe-commits mailing list cfe-commits@lis
Re: [PATCH] D16962: clang-tidy: avoid std::bind
alexfh added a comment. In http://reviews.llvm.org/D16962#346822, @jbcoe wrote: > Moved check to readability module. > > Aside: would it be worthwhile creating a python script to move checks from > one module to another? It's reasonable to teach the `rename_check.py` script move across modules. (Substantial comments later) http://reviews.llvm.org/D16962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16962: clang-tidy: avoid std::bind
jbcoe updated this revision to Diff 47259. jbcoe added a comment. Moved check to readability module. Aside: would it be worthwhile creating a python script to move checks from one module to another? http://reviews.llvm.org/D16962 Files: clang-tidy/readability/AvoidStdBindCheck.cpp clang-tidy/readability/AvoidStdBindCheck.h clang-tidy/readability/CMakeLists.txt clang-tidy/readability/ReadabilityTidyModule.cpp docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/readability-avoid-std-bind.rst test/clang-tidy/readability-avoid-std-bind.cpp Index: test/clang-tidy/readability-avoid-std-bind.cpp === --- /dev/null +++ test/clang-tidy/readability-avoid-std-bind.cpp @@ -0,0 +1,81 @@ +// RUN: %check_clang_tidy %s readability-avoid-std-bind %t -- -- -std=c++14 + +namespace std { +inline namespace impl { +template +class bind_rt {}; + +template +bind_rt bind(Fp&&, Arguments&& ...); +} +} + +int add(int x, int y) { return x + y; } + +void f() +{ + auto clj = std::bind(add,2,2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using std::bind [readability-avoid-std-bind] +} + +// CHECK-FIXES: auto clj = [] { return add(2, 2); }; + +void g() +{ + int x = 2; + int y = 2; + auto clj = std::bind(add,x,y); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using std::bind [readability-avoid-std-bind] +} + +// CHECK-FIXES: auto clj = [=] { return add(x, y); }; + +struct placeholder {}; +placeholder _1; +placeholder _2; + +void h() +{ + int x = 2; + auto clj = std::bind(add,x,_1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using std::bind [readability-avoid-std-bind] +} + +// CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); }; + +struct A; +struct B; +bool ABTest(const A&, const B&); + +void i() +{ + auto BATest = std::bind(ABTest, _2, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: avoid using std::bind [readability-avoid-std-bind] +} + +// CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); }; + +void j() +{ + auto clj = std::bind(add, 2, 2, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using std::bind [readability-avoid-std-bind] +} +// No fix is applied for argument mismatches. +// CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2); + +void k() +{ + auto clj = std::bind(add, _1, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using std::bind [readability-avoid-std-bind] +} +// No fix is applied for reused placeholders. +// CHECK-FIXES: auto clj = std::bind(add, _1, _1); + +void m() +{ + auto clj = std::bind(add, 1, add(2, 5)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using std::bind [readability-avoid-std-bind] +} +// No fix is applied for nested calls. +// CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5)); + Index: docs/clang-tidy/checks/readability-avoid-std-bind.rst === --- /dev/null +++ docs/clang-tidy/checks/readability-avoid-std-bind.rst @@ -0,0 +1,16 @@ +.. title:: clang-tidy - readability-avoid-std-bind + +readability-avoid-std-bind +=== + +Find uses of std::bind. Replace simple uses of std::bind with lambdas. Lambdas +will use value-capture where required. + +Right now it only handles free functions not member functions. + +Fixits are only generated for simple uses of std::bind. + +std::bind can be hard to read and can result in larger object files and +binaries due to type information that will not be produced by equivalent +lambdas. + Index: docs/clang-tidy/checks/list.rst === --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -79,6 +79,7 @@ modernize-use-override performance-for-range-copy performance-implicit-cast-in-loop + readability-avoid-std-bind readability-braces-around-statements readability-container-size-empty readability-else-after-return Index: clang-tidy/readability/ReadabilityTidyModule.cpp === --- clang-tidy/readability/ReadabilityTidyModule.cpp +++ clang-tidy/readability/ReadabilityTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "AvoidStdBindCheck.h" #include "BracesAroundStatementsCheck.h" #include "ContainerSizeEmptyCheck.h" #include "ElseAfterReturnCheck.h" @@ -31,6 +32,8 @@ class ReadabilityModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { +CheckFactories.registerCheck( +"readability-avoid-std-bind"); CheckFactories.registerCheck( "readability-braces-around-statements"); CheckFactories.registerCheck( Index: clang-tidy/readability/CMakeLists.txt === --- clang-tidy/readabil
Re: [PATCH] D16962: clang-tidy: avoid std::bind
jbcoe removed rL LLVM as the repository for this revision. jbcoe updated this revision to Diff 47247. jbcoe added a comment. Require C++14 for improved fixits. Do not attempt to generate fixits for more complicated uses of bind. http://reviews.llvm.org/D16962 Files: clang-tidy/misc/AvoidStdBindCheck.cpp clang-tidy/misc/AvoidStdBindCheck.h clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/misc-avoid-std-bind.rst test/clang-tidy/misc-avoid-std-bind.cpp Index: test/clang-tidy/misc-avoid-std-bind.cpp === --- /dev/null +++ test/clang-tidy/misc-avoid-std-bind.cpp @@ -0,0 +1,81 @@ +// RUN: %check_clang_tidy %s misc-avoid-std-bind %t -- -- -std=c++14 + +namespace std { +inline namespace impl { +template +class bind_rt {}; + +template +bind_rt bind(Fp&&, Arguments&& ...); +} +} + +int add(int x, int y) { return x + y; } + +void f() +{ + auto clj = std::bind(add,2,2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using std::bind [misc-avoid-std-bind] +} + +// CHECK-FIXES: auto clj = [] { return add(2, 2); }; + +void g() +{ + int x = 2; + int y = 2; + auto clj = std::bind(add,x,y); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using std::bind [misc-avoid-std-bind] +} + +// CHECK-FIXES: auto clj = [=] { return add(x, y); }; + +struct placeholder {}; +placeholder _1; +placeholder _2; + +void h() +{ + int x = 2; + auto clj = std::bind(add,x,_1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using std::bind [misc-avoid-std-bind] +} + +// CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); }; + +struct A; +struct B; +bool ABTest(const A&, const B&); + +void i() +{ + auto BATest = std::bind(ABTest, _2, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: avoid using std::bind [misc-avoid-std-bind] +} + +// CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); }; + +void j() +{ + auto clj = std::bind(add, 2, 2, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using std::bind [misc-avoid-std-bind] +} +// No fix is applied for argument mismatches. +// CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2); + +void k() +{ + auto clj = std::bind(add, _1, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using std::bind [misc-avoid-std-bind] +} +// No fix is applied for reused placeholders. +// CHECK-FIXES: auto clj = std::bind(add, _1, _1); + +void m() +{ + auto clj = std::bind(add, 1, add(2, 5)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using std::bind [misc-avoid-std-bind] +} +// No fix is applied for nested calls. +// CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5)); + Index: docs/clang-tidy/checks/misc-avoid-std-bind.rst === --- /dev/null +++ docs/clang-tidy/checks/misc-avoid-std-bind.rst @@ -0,0 +1,15 @@ +.. title:: clang-tidy - misc-avoid-std-bind + +misc-avoid-std-bind +=== + +Find uses of std::bind. Replace simple uses of std::bind with lambdas. Lambdas +will use value-capture where required. + +Right now it only handles free functions not member functions. + +Fixits are only generated for simple uses of std::bind. + +std::bind can result in larger object files and binaries due to type +information that will not be produced by equivalent lambdas. + Index: docs/clang-tidy/checks/list.rst === --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -46,6 +46,7 @@ misc-argument-comment misc-assert-side-effect misc-assign-operator-signature + misc-avoid-std-bind misc-bool-pointer-implicit-conversion misc-definitions-in-headers misc-inaccurate-erase Index: clang-tidy/misc/MiscTidyModule.cpp === --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -13,6 +13,7 @@ #include "ArgumentCommentCheck.h" #include "AssertSideEffectCheck.h" #include "AssignOperatorSignatureCheck.h" +#include "AvoidStdBindCheck.h" #include "BoolPointerImplicitConversionCheck.h" #include "DefinitionsInHeadersCheck.h" #include "InaccurateEraseCheck.h" @@ -48,6 +49,8 @@ "misc-assert-side-effect"); CheckFactories.registerCheck( "misc-assign-operator-signature"); +CheckFactories.registerCheck( +"misc-avoid-std-bind"); CheckFactories.registerCheck( "misc-bool-pointer-implicit-conversion"); CheckFactories.registerCheck( Index: clang-tidy/misc/CMakeLists.txt === --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -4,6 +4,7 @@ ArgumentCommentCheck.cpp AssertSideEffectCheck.cpp AssignOperatorSignatureCheck.cpp + AvoidStdBindCheck.cpp BoolPointerImplicitConversionCheck.cpp
Re: [PATCH] D16962: clang-tidy: avoid std::bind
jbcoe planned changes to this revision. jbcoe added a comment. Placeholder handling needs correcting. Repository: rL LLVM http://reviews.llvm.org/D16962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits