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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits