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

[Siva Chandra via Phabricator via libc-commits]

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
+  COMPILE_OPTIONS
+    -O3 # To get the effect of constexpr destructors of global objects.
   DEPENDS
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121350