[PATCH] D33209: [clang-tidy] Add "emplace_back" detection in inefficient-vector-operation.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:208
+           "consider pre-allocating the vector capacity before the loop")
+      << VectorAppendCall->getMethodDecl()->getDeclName();
 
----------------
hokein wrote:
> alexfh wrote:
> > Diagnostic builder should be able to format NamedDecls directly, this `->getDeclName()` is not necessary. The only difference is that it will likely add quotes around the name, which seems to be good anyway.
> I tried it, but I found the behavior between using `getDeclName()` and not using `getDeclName()` is different when handling the template functions:
> 
> * `diag(...) << VectorAppendCall->getMethodDecl()` will print the function name with instantiated template arguments like "emplace_back<int&>";
> *  `diag(...) << VectorAppendCall->getMethodDecl()->getDeclName()` will just print the function name without template arguments, which is what we expect.
Good to know.


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:50
+static const char PushBackOrEmplaceBackCallName[] =
+    "push_back_or_emplace_back_call";
 static const char LoopInitVarName[] = "loop_init_var";
----------------
nit: No need for the actual strings to be that long, since you're only using the corresponding constant names in the code. Even though there's no small string optimization used for bound AST nodes (they are stored in a `std::map<std::string, ast_type_traits::DynTypedNode>`), maps are frequently compared, which is less wasteful with shorter strings.

It would be interesting to see how large is the impact of longer or shorter string IDs, but until then we can still avoid overly long names.


================
Comment at: test/clang-tidy/performance-inefficient-vector-operation.cpp:158
+    std::vector<Foo> v;
+    // CHECK-FIXES: v.reserve(t.size());
+    for (const auto &e : t) {
----------------
This pattern is ambiguous. I'd use unique variable name for each test to avoid patterns matching incorrect lines.


https://reviews.llvm.org/D33209