[PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Stanisław Barzowski via cfe-commits]

sbarzowski added inline comments.

================
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:41
@@ +40,3 @@
+  // (and destructed) as in push_back case.
+  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
+      ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr", "std::auto_ptr",
----------------
vsk wrote:
> 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?
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.



http://reviews.llvm.org/D20964