[PATCH] D21185: [clang-tidy] Add performance-emplace-into-containers

[Samuel Benzaquen via cfe-commits]

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