This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add modernize-use-emplace
ClosedPublic

Authored by Prazek on Jun 3 2016, 7:45 AM.

Details

Summary

This check look for cases when inserting new element into an STL
container, but the element is constructed temporarily.

It changes it to call of emplace_back

Diff Detail

Repository
rL LLVM

Event Timeline

Prazek updated this revision to Diff 59555.Jun 3 2016, 7:45 AM
Prazek retitled this revision from to [clang-tidy] Add modernize-use-emplace.
Prazek updated this object.
Prazek added a reviewer: alexfh.
Prazek added a subscriber: staronj.
sbarzowski added inline comments.Jun 3 2016, 8:02 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
57 ↗(On Diff #59558)

capital letter?

docs/ReleaseNotes.rst
212 ↗(On Diff #59558)

typo: change -> changed

docs/clang-tidy/checks/modernize-use-emplace.rst
6 ↗(On Diff #59558)

Why "would"? Did you copy the description from the proposal? :-)

8 ↗(On Diff #59558)

Same here, and it is not obvious to me what is so special about std::pair here.

sbarzowski added inline comments.Jun 3 2016, 8:13 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
28 ↗(On Diff #59558)

Shouldn't it start with an uppercase letter?

sbarzowski added inline comments.Jun 3 2016, 8:13 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
37 ↗(On Diff #59558)

Same

41 ↗(On Diff #59558)

Same

44 ↗(On Diff #59558)

same

Prazek marked 4 inline comments as done.Jun 3 2016, 8:17 AM
Prazek added inline comments.
docs/clang-tidy/checks/modernize-use-emplace.rst
6 ↗(On Diff #59558)

yep :D

Prazek updated this revision to Diff 59561.Jun 3 2016, 8:19 AM
Prazek marked an inline comment as done.

Fixed stuff

Prazek updated this revision to Diff 59562.Jun 3 2016, 8:20 AM

Learning how to use arc

Prazek marked 4 inline comments as done.Jun 3 2016, 8:22 AM
Prazek added inline comments.
clang-tidy/modernize/UseEmplaceCheck.cpp
29 ↗(On Diff #59562)

I don't think so. In all checks people use lowerCamelCase, because it looks like a real matchers

Prazek marked 2 inline comments as done.Jun 3 2016, 8:22 AM

Could you describe in more detail (ideally in the original patch review summary/description) what this transformation does? Under what conditions does it suggest emplace, etc.

sbarzowski added inline comments.Jun 3 2016, 8:39 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
30 ↗(On Diff #59558)

Maybe it can work with some more types, with minimal changes... Like maybe std::list.

38 ↗(On Diff #59558)

This argument really holds for _any_ class taking ownership of the created object.

Maybe it would be better to check if we pass a non-const pointer as an argument to the constructor, as a pretty good indicator of taking ownership.

Or maybe just look for new operator in the expression.

Prazek added inline comments.Jun 3 2016, 8:51 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
31 ↗(On Diff #59562)

ok, std::list works for me. I just don't want to spend much time on it right now, and I want to be sure it works.

39 ↗(On Diff #59562)

Look at tests - the same thing happens when

std::vector<std::unique_ptr<int>> v;

const auto *p = new int;
v.push_back(p);

Not many custom classes take pointer in constructor.
If I will look for const pointers, then I will not be able to pass "abc" into vector<string>.

So I guess this solution seems to be the best idea.

Prazek updated this object.Jun 3 2016, 8:52 AM
Prazek updated this revision to Diff 59568.Jun 3 2016, 9:11 AM
  • Post review fix
sbarzowski added inline comments.Jun 3 2016, 9:25 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
40 ↗(On Diff #59568)

Look at tests - the same thing happens when

Yeah. I meant looking for new in addition to blacklist.

Not many custom classes take pointer in constructor.

Obviously depends on codebase, but IMO it's quite common. However usually this classes aren't copy-constructible (or at least shouldn't be) anyway, making it impossible to use push_back, so maybe it's not such a big deal.

If I will look for const pointers, then I will not be able to pass "abc" into vector<string>.

I wrote explicitly about only non-const pointers.

Prazek marked 2 inline comments as done.Jun 3 2016, 9:40 AM
Prazek added inline comments.
clang-tidy/modernize/UseEmplaceCheck.cpp
40 ↗(On Diff #59568)

It doesn't matter if it is const or not. Disabling any of them would disable some cases where it would be good.
I have to run it on llvm code base and see what happens

sbarzowski added inline comments.Jun 3 2016, 10:00 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
40 ↗(On Diff #59568)

Of course there would be some false negatives. It's close to impossible to know for sure if it's safe, so we need some sort of heuristic. My concern is that the current "blacklist smart pointers" solution may lead to a lot of false positives. And false positives here are quite nasty - very subtle bugs, that are unlikely to be caught by the tests or quick code review. So I'm more comfortable with false negatives.

I think will be good idea to try this check with LLVM STL too.

docs/clang-tidy/checks/modernize-use-emplace.rst
47 ↗(On Diff #59568)

Please highlight push_back with ``.

55 ↗(On Diff #59568)

Please highlight emplace_back with ``. Same below.

test/clang-tidy/modernize-use-emplace.cpp
12 ↗(On Diff #59568)

Please insert new line.

78 ↗(On Diff #59568)

Please insert new line.

Prazek updated this revision to Diff 59580.Jun 3 2016, 10:47 AM
Prazek marked 4 inline comments as done.
  • Review fixes

I think will be good idea to try this check with LLVM STL too.

You mean llvm::SmallVector stuff?

I think will be good idea to try this check with LLVM STL too.

You mean llvm::SmallVector stuff?

No, I meant to build example with -stdlib=libc++, -lc++, -lc++abi. Just to make sure, that hasName() is proper matcher.

Prazek added a comment.Jun 4 2016, 1:40 AM

I think will be good idea to try this check with LLVM STL too.

You mean llvm::SmallVector stuff?

No, I meant to build example with -stdlib=libc++, -lc++, -lc++abi. Just to make sure, that hasName() is proper matcher.

I runned it on llvm codebase with libstdc++ and it worked perfectly. I don't think there should be any change with libc++. I will run it only on small with libc++, because it will take too much time to compile whole llvm again.

There is alternative implementation in D21185. Will be good idea to make one which take the best from both :-)

vsk added a subscriber: vsk.Jun 9 2016, 1:12 PM

@Eugene.Zelenko thanks for pointing this out, I had totally missed this patch! Once we get this reviewed I'm willing to abandon my version. Some comments inline --

clang-tidy/modernize/UseEmplaceCheck.cpp
26 ↗(On Diff #59580)

cerfull -> careful

37 ↗(On Diff #59580)

becase -> because

test/clang-tidy/modernize-use-emplace.cpp
147 ↗(On Diff #59580)

I don't think this is correct. Comments immediately before/after S or immediately before/after the second right parenthesis should be preserved. C.f the implementation in D21185.

vsk added inline comments.Jun 9 2016, 6:21 PM
clang-tidy/modernize/UseEmplaceCheck.cpp
41 ↗(On Diff #59580)

I agree that blacklisting some smart pointers is not a complete solution, and that we shouldn't introduce a check which emits false positives.

ISTM it's only safe to perform the "push(T(...)) -> emplace(...)" change if: it's safe to assume that if "emplace(...)" does not successfully call "T(...)", it's OK for the program to fail/leak/crash. Do we get to make this assumption ever? Perhaps just in no-exceptions mode?

sbarzowski added inline comments.Jun 10 2016, 6:08 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
41 ↗(On Diff #59580)

hmmm... Maybe it's not that bad.

Let's look closely at the difference in behaviour.

with push_back it works like:

  1. Call the constructor
  2. Allocate the memory
  3. Call the copy constructor

And emplace_back is:

  1. Allocate the memory
  2. Call the constructor

Each of these steps in both scenarios may fail.

If we call the constructor and it fails, then it's alright. We have the same behaviour in both cases - if it can fail it should do its cleanup. (And failure of copy constructor obviously doesn't bother us).

So the scenario we have to worry about in only when memory allocation fails. Then with push_back, we'll call constructor and then destructor, while emplace_back doesn't call constructor at all.

So the real question is: "Is it safe to throw some exception and not to call the constructor at all". And I see only one real-world case it isn't - taking the ownership of the object.

vsk added inline comments.Jun 10 2016, 4:19 PM
clang-tidy/modernize/UseEmplaceCheck.cpp
41 ↗(On Diff #59580)

@sbarzowski Right, I think we're in agreement about what the problem is. I just don't think there are heuristics or comprehensive blacklists to detect classes which have ownership semantics. E.g, consider a FileHandle class which owns a file descriptor and is responsible for closing it.

Prazek updated this revision to Diff 60662.Jun 14 2016, 4:08 AM
Prazek marked an inline comment as done.

Runned on LLVM and bug fixed one thing

Prazek added a subscriber: hokein.
hokein edited edge metadata.Jun 17 2016, 7:58 AM

A few comments.

clang-tidy/modernize/ModernizeTidyModule.cpp
59 ↗(On Diff #60662)

Alphabetical order.

clang-tidy/modernize/UseEmplaceCheck.cpp
37 ↗(On Diff #60662)

remove the will here? if emplacement fails, we will ...

78 ↗(On Diff #60662)

missing . at the end of sentence.

clang-tidy/modernize/UseEmplaceCheck.h
19 ↗(On Diff #60662)

s/look/looks

docs/clang-tidy/checks/modernize-use-emplace.rst
6 ↗(On Diff #60662)

s/look/looks

test/clang-tidy/modernize-use-emplace.cpp
2 ↗(On Diff #60662)

Could you also add the following case in the test?

vector<vector<int>> v1;
v1.push_back(1<<10);

Make sure the check won't change to v1.emplace_back(1<<10). Since these two statements have completely different semantic. The first one adds 1024 to v1 while the second one adds a vector with 1024 elements to v1.

94 ↗(On Diff #60662)

I think you can remove the {{..}}.

Prazek added inline comments.Jun 17 2016, 9:37 AM
test/clang-tidy/modernize-use-emplace.cpp
2 ↗(On Diff #60662)

I think you have made some mistake, because the code that you have showed doesn't compile.

Prazek marked 8 inline comments as done.Jun 17 2016, 4:44 PM
Prazek updated this revision to Diff 61143.Jun 17 2016, 4:45 PM
Prazek updated this object.
Prazek edited edge metadata.
Prazek added a reviewer: sbenza.

You might be interested.

Prazek updated this revision to Diff 61214.Jun 19 2016, 12:37 PM

Changes after running og llvm code base. Adding llvm::SmallVector was a good idea, because it has fired for much more cases and I have found 3 other bugs.
Now check doesn't fire when argument of constructor is bitfield or new expression, or when constructor is converted to base class.
I will post diff of changes after running in a minute

In D20964#453797, @vsk wrote:

@Eugene.Zelenko thanks for pointing this out, I had totally missed this patch! Once we get this reviewed I'm willing to abandon my version. Some comments inline --

I have tested it on llvm code base and I have found many, many corner cases. I see that your check have better fixit messages (it only prints change of push_back -> emplace_back). I guess the best idea is to push this check, and then if you would like, change it so it would also have nice fixit messages

hokein accepted this revision.Jun 20 2016, 5:04 AM
hokein edited edge metadata.

Looks almost good now, a few comments. You'd better await for comments from @alexfh or @sbenza before committing.

docs/clang-tidy/checks/modernize-use-emplace.rst
62 ↗(On Diff #61214)

s/fire/fires
s/woule/would

test/clang-tidy/modernize-use-emplace.cpp
3 ↗(On Diff #61214)

Oops, you are right. That's another non-relevant issue (different emplace_back behaviors in vector<int>, vector<vector<int>>). I'm over-concerned here.

317 ↗(On Diff #61214)

Looks like the test is kind of duplicated with testAggregate in L248, you can merge that one into this one.

This revision is now accepted and ready to land.Jun 20 2016, 5:04 AM
This revision was automatically updated to reflect the committed changes.
sbenza added inline comments.Jun 21 2016, 11:46 AM
clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
34

We should not hard code this list. Specially not non-standard types like llvm::SmallVector.
This should come from an option string.
For example, like we do in performance/FasterStringFindCheck.cpp

42

These are not the only smart pointers around.
It might be a good idea to make this list also configurable by the user.

45

There are a few more things that can break here:

  • NULL can't be passed through perfect forwarding. Will be deduced as long.
  • Function references/pointers can't be passed through PF if they are overloaded.
  • Class scope static variables that have no definition can't be passed through PF because they will be ODR used.
92

You should avoid using offsets with locations.
For example, you are hardcoding { as one character, which might not be true in the case of digraphs.

This should be getTokenRange(InnerCtorCall->getExprLoc(), CallParensRange.getBegin())

Same thing for the other one, it should be:

CharSourceRange::getTokenRange(CallParensRange.getEnd(), CallParensRange.getEnd())

clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.h
23

runaway qoute

clang-tools-extra/trunk/clang-tidy/utils/Matchers.h
48

This matcher is generic enough that it should go into main ASTMatchers.h file.

clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst
17

Don't add the space between the >>. This is not needed since we are in C++11.

52

This doesn't compile. unique_ptr is not implicitly convertible from a pointer.

53

I think you meant auto* ptr = new int(5) ?

clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp
76

You should also try with a type that has a user-defined destructor.
It changes the AST enough to make a difference in many cases.

And you should fix all the std classes above to have user-defined constructors too.

209

This test is broken. unique_ptr<T> should not have an implicit conversion from T*.

338

It is also missing tests with templates.
ie: the container being a dependent type, and the value_type being a dependent type.
We should not change the code in either of those cases.

Thank you Samuel, that it is valuable review!
I wanted to add the functionality of specifing stuff by options later.

should we revert this patch? It seems that those bugs are not so critical (at least on llvm http://reviews.llvm.org/D21507).
Anyway, I will fix it in one week.