[libc-commits] [PATCH] D125966: [libc][mem*] Address facility + test enum support

[Clement Courbet via Phabricator via libc-commits]

courbet added inline comments.


================
Comment at: libc/src/string/memory_utils/address.h:24
+template <bool flag = false>
+static void deferred_static_assert(const char *msg) {
+  static_assert(flag, "compilation error");
----------------
`DeferredStaticAssert` for consistency ? Note that as duscussed offline, I would much rather prefer consistency with c++, i.e. renaming everything to `cpp::conditional_t` instead of `cpp:ConditionalType`.


================
Comment at: libc/src/string/memory_utils/address.h:31
+
+// Address attribute instructing whether the underlying load / store operations
+// are temporal or non-temporal.
----------------
"specifying" ? (here and below).


================
Comment at: libc/src/string/memory_utils/address.h:33
+// are temporal or non-temporal.
+enum class Temporal { NO, YES };
+
----------------
I would call this `Temporality { TEMPORAL, NONTEMPORAL}`. Else `AddrT::TEMPORAL` reads as "a temporal address" rather than "the temporality of the `Address` type `AddrT`".


================
Comment at: libc/src/string/memory_utils/address.h:47
+  static_assert(is_power2(Alignment));
+  static constexpr bool IS_ADDRESS_TYPE = true;
+  static constexpr size_t ALIGNMENT = Alignment;
----------------
Not a big fan of this, because this is spoofable: `struct SomeOtherType { static constexpr bool IS_ADDRESS_TYPE = true; };`. Is this on purpose ? Else I think `template <typename T> IsAddressType { constexpr bool value = false; };` + specialization would be better.


================
Comment at: libc/src/string/memory_utils/address.h:53
+  static constexpr bool IS_WRITE = P == Permission::Write;
+  using value_type = cpp::ConditionalType<!IS_WRITE, const ubyte, ubyte>;
+  using void_type = cpp::ConditionalType<!IS_WRITE, const void, void>;
----------------
`PointeeType` ? I could see `ValueType` being `ubyte*` or `ubyte`. Using `Pointee` makes it non-ambiguous.


================
Comment at: libc/src/string/memory_utils/address.h:54
+  using value_type = cpp::ConditionalType<!IS_WRITE, const ubyte, ubyte>;
+  using void_type = cpp::ConditionalType<!IS_WRITE, const void, void>;
+
----------------
`VoidType` ?


================
Comment at: libc/src/string/memory_utils/address.h:66
+
+// Lowers the address to a pointer of type T.
+template <typename T, typename AddrT> static T *as(AddrT addr) {
----------------
Maybe mention that this is not UB because `value_type` is a character type ? This triggers alarms in my brain :)


================
Comment at: libc/src/string/memory_utils/address.h:74
+// alignment whenever possible.
+template <size_t Offset, typename AddrT> static auto offsetAddr(AddrT addr) {
+  static_assert(AddrT::IS_ADDRESS_TYPE);
----------------
The doc should mention whether the offsets are in bytes or multiple of alignment. As of now I have to read the code to figure out.


================
Comment at: libc/src/string/memory_utils/address.h:76
+  static_assert(AddrT::IS_ADDRESS_TYPE);
+  constexpr size_t AL = Offset < AddrT::ALIGNMENT ? Offset : AddrT::ALIGNMENT;
+  return Address<AL, AddrT::PERMISSION, AddrT::TEMPORAL>(addr.ptr_ + Offset);
----------------
I don't understand why an address offset by `AddrT::ALIGNMENT+1` should be `AddrT::ALIGNMENT`-aligned. is this missing a `static_assert(Offset <= AddrT::ALIGNMENT)` ?


================
Comment at: libc/src/string/memory_utils/address.h:89
+
+// Offsets the address by a runtime amount that is assumed to be a multiple of
+// Offset. This allows to propagate the address alignement whenever possible.
----------------
(in bytes)


================
Comment at: libc/src/string/memory_utils/address.h:94
+  static_assert(AddrT::IS_ADDRESS_TYPE);
+  constexpr size_t AL = Offset < AddrT::ALIGNMENT ? Offset : AddrT::ALIGNMENT;
+  return offsetAddrAssumeAligned<AL>(addr, offset);
----------------
ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125966