[libcxx-commits] [PATCH] D115368: [NFC][libcxxabi] Re-organized inheritance structure to remove CRTP in cxa_guard

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxxabi/src/cxa_guard_impl.h:212-215
+  /// The init byte portion of cxa_guard_acquire. Returns true if
+  /// initialization has already been completed.
+  /// Note: On completion, we haven't 'acquired' ownership of anything mutex
+  /// related. The name is simply referring to __cxa_guard_acquire.
----------------
The duplication of these comments feels weird to me. Instead, I believe we should have a comment like:

```
//===----------------------------------------------------------------------===//
//                    Initialization Byte Implementations
//===----------------------------------------------------------------------===//
//
// Each initialization byte implementation supports the following method:
//
//    InitByte(uint8_t* _init_byte_address, uint32_t*)
//        Here explain the purpose of these constructors.
//
//    bool acquire();
//        Here document what this method does and you can have the note about ownership documented in exactly one place, here.
//
//    etc...
//
```

Then, each implementation of an initialization byte can simply implement the required methods and document anything interesting that is specific to that implementation.


================
Comment at: libcxxabi/src/cxa_guard_impl.h:282
+      : init_byte_address(_init_byte_address), thread_id_address(_thread_id_address),
+        has_thread_id_support(_thread_id_address != nullptr && GetThreadID != nullptr) {}
 
----------------
With this refactor, it now feels really weird to have this `GetThreadID != nullptr` here. Would it be possible to move the determination of whether the thread id is supported elsewhere (probably in the `GuardObject` constructor) so that `InitByteGlobalMutex` only has to check `_thread_id_address != nullptr`?

Same applies to Futex below.


================
Comment at: libcxxabi/src/cxa_guard_impl.h:406-409
+      : init_byte(_init_byte_address),
+        has_thread_id_support(_thread_id_address != nullptr && GetThreadIDArg != nullptr),
+        thread_id(_thread_id_address), base_address(reinterpret_cast<int*>(
+                                           /*_init_byte_address & ~0x3*/ _init_byte_address - 1)) {}
----------------
Also, can you explain the `/*_init_byte_address & ~0x3*/` comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115368