sbenza added a comment. Missing the .rst file. Did you use the check_clang_tidy.py script to create the template?
================ Comment at: clang-tidy/performance/EmplaceCheck.cpp:25 @@ +24,3 @@ + cxxMemberCallExpr( + on(expr(hasType(cxxRecordDecl(hasName("std::vector"))))), + callee(functionDecl(hasName("push_back"))), ---------------- This change can potentially break exception guarantees of the code. For example, push_back can throw before the object is constructed leaking the arguments. We should at least say something about it in the docs. ================ Comment at: clang-tidy/performance/EmplaceCheck.cpp:26 @@ +25,3 @@ + on(expr(hasType(cxxRecordDecl(hasName("std::vector"))))), + callee(functionDecl(hasName("push_back"))), + hasArgument(0, cxxConstructExpr().bind("construct_expr"))) ---------------- Just say functionDecl(hasName("std::vector::push_back")) and you can remove the type check. ================ Comment at: clang-tidy/performance/EmplaceCheck.cpp:84 @@ +83,3 @@ + + StringRef ArgsStr = getAsString(SM, LO, ArgsR); + if (!ArgsStr.data()) ---------------- It's usually preferable, and sometimes easier to write, to do erases instead of replacements. For example, you could simply erase `X(` and `)`. Something like `(Cons->getLogStart(), ArgsR.getBegin())` and `(Args.getEnd(), Args.getEnd())` ================ Comment at: clang-tidy/performance/EmplaceCheck.cpp:88 @@ +87,3 @@ + + SourceRange CalleeR = getPreciseTokenRange(SM, LO, CalleeStartLoc); + SourceRange ConstructR = Cons->getSourceRange(); ---------------- What you want is `getTokenRange(Call->getCallee()->getSourceRange())` No need to mess with the locations and offsets yourself. ================ Comment at: clang-tidy/performance/EmplaceCheck.h:18 @@ +17,3 @@ +namespace performance { + +class EmplaceCheck : public ClangTidyCheck { ---------------- Needs a comment ================ Comment at: clang-tidy/performance/EmplaceCheck.h:19 @@ +18,3 @@ + +class EmplaceCheck : public ClangTidyCheck { +public: ---------------- What's the scope? Is it just vector::push_back/emplace_back? Or is it going to be expanded to other map::insert, deque::push_front, etc. ================ Comment at: test/clang-tidy/performance-emplace-into-containers.cpp:26 @@ +25,3 @@ + std::vector<X> v1; + + v1.push_back(X()); ---------------- We have to make sure the arguments are compatible with perfect forwarding. This means no brace init lists and no NULL. Eg: v.push_back(Y({})); // {} can't be passed to perfect forwarding v.push_back(Y(NULL)); // NULL can't be passed to perfect forwarding ================ Comment at: test/clang-tidy/performance-emplace-into-containers.cpp:28 @@ +27,3 @@ + v1.push_back(X()); + // CHECK-MESSAGES: [[@LINE-1]]:6: warning: Consider constructing {{.*}} + // CHECK-FIXES: v1.emplace_back() ---------------- At least one of the cases should match the whole exact message, to make sure it is correct. ================ Comment at: test/clang-tidy/performance-emplace-into-containers.cpp:42 @@ +41,3 @@ + // CHECK-FIXES: v1/*a*/./*b*/emplace_back(/*c*//*d*/0,/*e*/0/*f*//*g*/)/*h*/ ; + + // Do not try to deal with initializer lists. ---------------- We should also test implicit conversions. Eg: v1.push_back(0); ================ Comment at: test/clang-tidy/performance-emplace-into-containers.cpp:46 @@ +45,3 @@ + + // Do not try to deal with functional casts. FIXME? + X x{0, 0}; ---------------- Why not? ================ Comment at: test/clang-tidy/performance-emplace-into-containers.cpp:50 @@ +49,3 @@ + v1.push_back(X(0)); + + // Do not try to deal with weird expansions. ---------------- We need tests for copy/move construction. Eg: v1.push_back(x); // should not be changed v1.push_back(FunctionThatReturnsX()); // should not be changed http://reviews.llvm.org/D21185 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits