This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add "emplace_back" detection in inefficient-vector-operation.
ClosedPublic

Authored by hokein on May 15 2017, 12:32 PM.

Event Timeline

hokein created this revision.May 15 2017, 12:32 PM
Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a project: Restricted Project.
malcolm.parsons added inline comments.
clang-tidy/performance/InefficientVectorOperationCheck.cpp
92

I'd use ignoringImplicit(VectorAppendCallExpr) to ignore ExprWithCleanups.

alexfh added inline comments.May 16 2017, 12:15 AM
clang-tidy/performance/InefficientVectorOperationCheck.cpp
207

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.

hokein updated this revision to Diff 99119.May 16 2017, 1:58 AM
hokein marked an inline comment as done.

Adress review comments.

hokein marked an inline comment as done.May 16 2017, 2:00 AM
hokein added inline comments.
clang-tidy/performance/InefficientVectorOperationCheck.cpp
92

Good to know. Thanks!

207

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.
alexfh added inline comments.May 16 2017, 2:46 AM
clang-tidy/performance/InefficientVectorOperationCheck.cpp
50

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.

207

Good to know.

test/clang-tidy/performance-inefficient-vector-operation.cpp
158

This pattern is ambiguous. I'd use unique variable name for each test to avoid patterns matching incorrect lines.

alexfh accepted this revision.May 16 2017, 2:48 AM

LG once the comments are addressed.

This revision is now accepted and ready to land.May 16 2017, 2:48 AM
hokein updated this revision to Diff 99124.May 16 2017, 3:33 AM
hokein marked 3 inline comments as done.

Improve tests.

clang-tidy/performance/InefficientVectorOperationCheck.cpp
50

Acknowledged. Thanks for the detailed explanation ;)

test/clang-tidy/performance-inefficient-vector-operation.cpp
158

Sounds good. Also applied the change to the whole test file.

This revision was automatically updated to reflect the committed changes.