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

Siva Chandra via Phabricator via libc-commits libc-commits at lists.llvm.org
Thu Mar 10 01:17:03 PST 2022

sivachandra added inline comments.

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 {
abrachet wrote:
> 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`?
If we have to support bidirectional iteration, then we need a doubly-linked list. Not sure if we will ever need bidirectional iteration so I have chosen to keep it a simple linked list with the direction attached to the type.

> Maybe it makes more sense for `BlobStore<T>::iterator` to have `REVERSE_ORDER`?

Do you mean, it enables range-for-loop? Or, something else?

Comment at: libc/src/stdlib/CMakeLists.txt:280
+    -O3 # To get the effect of constexpr destructors of global objects.
abrachet wrote:
> 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>)`
You are correct. I think I misread or checked it with the previous mutex handling still present that I did see a call to `__cxa_atexit`. I don't see it now.

That said, in a debug build, I surprisingly see a call to the constructor (via `__cxx_global_var_init`) if I don't use -O3 [may be -O2 is sufficient, I did not check]. In a release build, everything works as expected without -O3 - I don't see calls to the constructor or `__cxa_atexit`. So, I have kept the -O3 for now.

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;
abrachet wrote:
> If we keep `BlobStore` with `REVERSE_ORDER` this could just be `for (auto callback : exit_callbacks)`
Ah, yes. Done now.

  rG LLVM Github Monorepo



More information about the libc-commits mailing list