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

[Alex Brachet via Phabricator via libc-commits]

abrachet added inline comments.


================
Comment at: libc/src/__support/CPP/blobstore.h:1
+//===-- A data structure which stores data in blobs  ------------*- C++ -*-===//
+//
----------------
Do you think the name `BlockStore` better describes what this data structure does? It may not be worth the churn to change the name everywhere, no strong preference.


================
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 {
----------------
sivachandra wrote:
> 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?
> 
No strong preference because we would need to add one pointer overhead with a new `prev` pointer to `Blob`, but it would be much more ergonomic if `BlobStore` could iterate both ways.

`REVERSE_ORDER` could be changed to only be used in the iterator. So maybe it could be like
```
template <bool REVERSE_ORDER> class iterator_impl { ... };
using iterator = iterator_impl<false>;
using reverse_iterator = iterator_impl<true>;
iterator rbegin() {
  return reverse_iterator(current, fill_count);
}
...
```



================
Comment at: libc/src/stdlib/CMakeLists.txt:280
+  COMPILE_OPTIONS
+    -O3 # To get the effect of constexpr destructors of global objects.
   DEPENDS
----------------
sivachandra wrote:
> 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.
> 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
We just bumped up to C++17 and we already need C++20's `constinit`! :)

Specifying `O3` here makes this TU basically impossible to debug and generally limits options. Maybe we want to optimize hard for size with `Oz` but atexit will always be `O3`.

Are global constructors something we need to avoid? The destructor was going to affect correctness, but unless I'm missing something the same isn't true for constructors. Is it worth the downside above if this will be optimized away during release builds anyway?



================
Comment at: libc/src/stdlib/atexit.cpp:22
+using ExitCallbackList = cpp::ReverseOrderBlobStore<AtExitCallback *, 32>;
+static ExitCallbackList exit_callbacks;
 
----------------
I don't know why I made this `static` before when it is already in an anonymous namespace


================
Comment at: libc/src/stdlib/atexit.cpp:30
   handler_list_mtx.lock();
-  // TODO: implement rbegin() + rend() for cpp::vector
-  for (int i = handlers.size() - 1; i >= 0; i--) {
+  auto end = exit_callbacks.end();
+  for (auto callback : exit_callbacks) {
----------------
Not needed anymore


================
Comment at: libc/src/stdlib/atexit.cpp:41
 
-LLVM_LIBC_FUNCTION(int, atexit, (void (*function)())) {
+LLVM_LIBC_FUNCTION(int, atexit, (AtExitCallback * callback)) {
   handler_list_mtx.lock();
----------------
I think this is how we would normally format this. But if `clang-format` get's confused because of the parentheses then we should just go with what `clang-format` says.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121350