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

[Vedant Kumar via cfe-commits]

vsk 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",
----------------
sbarzowski wrote:
> 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.
> 
@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.


http://reviews.llvm.org/D20964