[libcxx-commits] [PATCH] D110093: [NFC][libcxxabi] In cxa_guard, split the InitByte classes into InitByte... and ...Guard

[Daniel McIntosh via Phabricator via libcxx-commits]

DanielMcIntosh-IBM added inline comments.


================
Comment at: libcxxabi/src/cxa_guard_impl.h:174
 template <class Derived>
 struct GuardObject {
   GuardObject() = delete;
----------------
ldionne wrote:
> I was looking at this patch and I found that using CRTP for this was adding a lot of complexity. It it possible that using a free function (template) for implementing `cxa_guard_acquire`, `cxa_guard_release` and `cxa_guard_abort` would greatly simplify things? Imagine:
> 
> ```
> template <class Guard>
> AcquireResult cxa_guard_acquire(Guard& guard) {
>   AtomicInt<uint8_t> guard_byte(guard.get_init_byte()); // all types of guards need to answer to that
>   if (guard_byte.load(std::_AO_Acquire) != UNSET)
>     return INIT_IS_DONE;
>   return derived()->acquire_init_byte();
> }
> 
> // etc..
> ```
> 
> Then, for example `NoThreadsGuard` would become:
> 
> ```
> using NoThreadsGuard = InitByteNoThreads;
> ```
> 
> At that point we might want to simplify the whole thing and simply call `InitByteNoThreads` `NoThreadsGuard` directly.
> 
Yes, using CRTP is one of the reasons I found the current implementation a little difficult to follow, which prompted this whole refactor. I get rid of it in D110101. I'm not entirely sure if I fully understand what it is you're suggesting, but I think that change accomplishes the same goal, except they're part of `GuardBase` instead of free functions. This works a bit better with the `SelectImplementation` stuff at the end of the file. Plus, I think having separate classes dedicated to managing the guard byte, to managing the init byte, and to co-coordinating between them is probably a little better for separation of concerns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110093