[libc-commits] [PATCH] D121350: [libc] Add a resizable container with constexpr constructor and destructor.

[Alex Brachet via Phabricator via libc-commits]

abrachet added a comment.

> The new container is used to store atexit callbacks. This way, we avoid
> the possibility of the destructor of the container itself getting added
> as at exit callback.

Ah I didn't think about that, we would run into this when we implement `__cxa_atexit`. That would have been a confusing sanitizer error! Good catch.



================
Comment at: libc/src/__support/CPP/blobstore.h:31
+// on its value will be optimized out in the code below.
+template <typename T, size_t BLOB_SIZE, bool REVERSE_ORDER = false>
+class BlobStore {
----------------
Could we just have a `reverse_iterator [rbegin|rend]()` instead of this `REVERSE_ORDER` template parameter? Maybe it makes more sense for `BlobStore<T>::iterator` to have `REVERSE_ORDER`?


================
Comment at: libc/src/stdlib/CMakeLists.txt:280
+  COMPILE_OPTIONS
+    -O3 # To get the effect of constexpr destructors of global objects.
   DEPENDS
----------------
>From what I understand, as long as `T` is POD and `BlobStore<T>::~BlobStore() is defaulted, then there shouldn't be a destructor emitted because then BlobStore will be a POD too. Do you see `__cxa_atexit` being called in this TU?

We could in the future when we add an analogue to `is_trivially_destructible` in our type_traits, add `static_assert(cpp::IsTriviallyDestructibleV<ExitCallbackList>)`


================
Comment at: libc/src/stdlib/atexit.cpp:31
+  auto end = exit_callbacks.end();
+  for (auto iter = exit_callbacks.begin(); iter != end; ++iter) {
+    auto callback = *iter;
----------------
If we keep `BlobStore` with `REVERSE_ORDER` this could just be `for (auto callback : exit_callbacks)`


================
Comment at: libc/src/stdlib/atexit.cpp:37
   }
+  ExitCallbackList::destroy(&exit_callbacks);
 }
----------------
It may not be worth deallocating this when we are going to exit soon anyway. But it might not be worth the trouble of needing to make sanitizers ignore this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121350