[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 abandoned this revision.
DanielMcIntosh-IBM added inline comments.


================
Comment at: libcxxabi/src/cxa_guard_impl.h:174
 template <class Derived>
 struct GuardObject {
   GuardObject() = delete;
----------------
DanielMcIntosh-IBM wrote:
> 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.
Actually, looking at this with fresh eyes, I think there is a better way of grouping these changes. See D115367, D115368 and D115369 for newer versions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110093