This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add performance-emplace-into-containers
AbandonedPublic

Authored by vsk on Jun 9 2016, 9:07 AM.

Details

Summary

Introduce a check which suggests when it might be helpful to use "emplace" methods. The initial version only works with std::vector and push_back (e.g V.push_back(T(1, 2)) -> V.emplace_back(1, 2)). It could be extended to work with other containers and methods.

Diff Detail

Event Timeline

vsk updated this revision to Diff 60183.Jun 9 2016, 9:07 AM
vsk retitled this revision from to [clang-tidy] Add performance-emplace-into-containers.
vsk updated this object.
vsk added reviewers: aaron.ballman, alexfh.
vsk added a subscriber: cfe-commits.
vsk updated this revision to Diff 60194.Jun 9 2016, 10:18 AM
vsk added a reviewer: flx.
  • Fix handling of push_back(X(/* comment */)).
  • Don't try to emit a warning when the callee comes from a macro expansion.
  • Get rid of a weird/wrong comment about checking for "default constructors".

As a side note, I'd appreciate advice on how to apply this check to the llvm codebase (via some kind of cmake invocation?) and gather numbers.

vsk updated this revision to Diff 60206.Jun 9 2016, 11:04 AM
  • Fix the diagnostic message. Suggested by Erik Pilkington.

See also D20964. I think modernize is better place for such check.

alexfh edited reviewers, added: sbenza; removed: flx, alexfh.Jun 17 2016, 5:15 AM
sbenza edited edge metadata.Jun 17 2016, 11:52 AM

Missing the .rst file.
Did you use the check_clang_tidy.py script to create the template?

clang-tidy/performance/EmplaceCheck.cpp
25

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.

26

Just say functionDecl(hasName("std::vector::push_back")) and you can remove the type check.

84

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())

88

What you want is getTokenRange(Call->getCallee()->getSourceRange())
No need to mess with the locations and offsets yourself.

clang-tidy/performance/EmplaceCheck.h
18

Needs a comment

19

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.

test/clang-tidy/performance-emplace-into-containers.cpp
26

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
28

At least one of the cases should match the whole exact message, to make sure it is correct.

42

We should also test implicit conversions.
Eg:

v1.push_back(0);
46

Why not?

50

We need tests for copy/move construction.
Eg:

v1.push_back(x);  // should not be changed
v1.push_back(FunctionThatReturnsX());  // should not be changed
aaron.ballman added inline comments.Jun 22 2016, 6:19 AM
clang-tidy/performance/EmplaceCheck.cpp
26

Should use "::std::vector::push_back" just to be sure it doesn't explode when evil people have:

namespace frobble {
namespace std {
template <typename T>
class vector {
  void push_back(const T&);
};
}
}
38

I think this goes against our coding standards and should be return StringRef();.

Since D20964 was committed, I think we should close this.

Prazek added a subscriber: Prazek.Jun 22 2016, 9:15 AM

Since D20964 was committed, I think we should close this.

Yep, I think the best idea is to take all the goodies from this check and add it to modernize-use-emplace

vsk abandoned this revision.Jun 22 2016, 9:55 AM

Thanks for the valuable feedback! I'll keep all of it in mind when writing checks in the future.

I'm closing this revision since D20964 is in.