alexfh added inline comments. ================ Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:37 @@ +36,3 @@ + +std::vector<BindArgument> +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<BindArgument> +buildBindArguments(const MatchFinder::MatchResult &Result, const CallExpr *C) { + std::vector<BindArgument> 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<MaterializeTemporaryExpr>(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<BindArgument> &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<BindArgument> &Args) { + std::unordered_map<size_t, size_t> PlaceHolderIndexCounts; + for (const BindArgument &B : Args) { ---------------- llvm::SmallPtrSet<size_t> 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<CallExpr>("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@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits