[libcxx-commits] [PATCH] D138601: [libc++] Fix memory leaks when throwing inside std::vector constructors

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/vector:297
 #include <__memory/temp_value.h>
 #include <__memory/uninitialized_algorithms.h>
 #include <__memory_resource/polymorphic_allocator.h>
----------------
In the bug report, you mention this was introduced when we removed the base class. Are you referring to b82da8b555603b172f826fdd5ea902eff3aeb7ad?

I looked at the commit and I still don't see how that introduced this issue (but I do acknowledge that this is a real issue, and a somewhat embarrassing one at that).


================
Comment at: libcxx/include/vector:1070
 {
     std::__debug_db_insert_c(this);
     if (__n > 0)
----------------
This comment applies to all the constructors. If `__vallocate()` fails, we won't call the destructor, so we won't remove `*this` from the debug database.

That being said, the legacy debug mode is already broken pretty bad so I'm not sure we can or want to fix it.


================
Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/throwing.pass.cpp:11
+
+// Check that vector constructors don't leak memory when an operation inside the constructor throws an exception
+
----------------
Please add a link to the bug report.


================
Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/throwing.pass.cpp:92
+  using AllocVec = std::vector<int, Allocator<int> >;
+  try { // Throw in default constructor
+    AllocVec vec;
----------------
Instead, I would list the signatures of the constructors you're testing. It will make it clearer what's going on at a glance.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138601/new/

https://reviews.llvm.org/D138601