[libcxx-commits] [PATCH] D140278: [libc++] Add __pointer_int_pair

[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: libcxx/include/__math_h/log2i.h:9
+
+#ifndef _LIBCPP___MATH_LOG2I_H
+#define _LIBCPP___MATH_LOG2I_H
----------------
Why is this not just under `__math`?


================
Comment at: libcxx/include/__math_h/log2i.h:23
+template <typename _Number>
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _Number __log2i(_Number __n) {
+  if (__n == 0)
----------------
We might as well add a comment explaining what the function does (compute the base-2 log of an integral type)?


================
Comment at: libcxx/include/__math_h/log2i.h:27
+  if (sizeof(__n) <= sizeof(unsigned))
+    return sizeof(unsigned) * CHAR_BIT - 1 - __libcpp_clz(static_cast<unsigned>(__n));
+  if (sizeof(__n) <= sizeof(unsigned long))
----------------
Here and elsewhere?


================
Comment at: libcxx/include/__utility/pointer_int_pair.h:62-64
+template <class _Pointer,
+          size_t __int_bit_count,
+          class _IntType       = size_t,
----------------
I really feel like the parameters should be the other way around here, because this is really a pair, and we always specify both types in a pair. I suggest we do this:

```
enum class __bitfield_width : size_t { };

template <class _Pointer, class _IntType, __bitfield_width __int_bit_count>
struct __pointer_int_pair;
```

This way, we can do this, which is more obvious than what we have right now IMO:

```
__pointer_int_pair<Overaligned*, size_t, __bitfield_width(2)>
```

In particular, I don't see why we have a default `size_t` type for the integral type. I don't think it makes sense, since this tries to be a general pointer-and-integral-type pair.


================
Comment at: libcxx/include/__utility/pointer_int_pair.h:65
+          class _IntType       = size_t,
+          class _PointerTraits = _PointerLikeTraits<_Pointer>>
+class __pointer_int_pair {
----------------
I think this can be defined as a typedef inside the class?


================
Comment at: libcxx/include/__utility/pointer_int_pair.h:79-84
+  __pointer_int_pair()                                     = default;
+  __pointer_int_pair(const __pointer_int_pair&)            = default;
+  __pointer_int_pair(__pointer_int_pair&&)                 = default;
+  __pointer_int_pair& operator=(const __pointer_int_pair&) = default;
+  __pointer_int_pair& operator=(__pointer_int_pair&&)      = default;
+  ~__pointer_int_pair()                                    = default;
----------------
`_LIBCPP_HIDE_FROM_ABI`


================
Comment at: libcxx/include/__utility/pointer_int_pair.h:86
+
+  _LIBCPP_HIDE_FROM_ABI __pointer_int_pair(_Pointer __ptr_value, _IntType __int_value)
+      : __value_(_PointerTraits::__to_uintptr(__ptr_value) | (__int_value << __extra_bits)) {
----------------
Please add a test that uses the implicit syntax here to lock in this behavior. I think it's useful to have this be non-explicit so you can do something like `return {ptr, int}` when you know that the return type is a `__pointer_int_pair`, but I think it should be locked with a test.


================
Comment at: libcxx/include/__utility/pointer_int_pair.h:102-103
+
+template <class _Pointer>
+using __pointer_bool_pair = __pointer_int_pair<_Pointer, 1, bool>;
+
----------------
I would hold off creating this until you need it (which may be in the very next patch, but then we can see it in context and decide whether it pulls its weight).


================
Comment at: libcxx/include/__utility/pointer_int_pair.h:109-110
+  using _PointerIntPair = __pointer_int_pair<_Pointer, __int_bit_count, _IntType, _PointerTraits>;
+  static constexpr size_t __pointer_low_bits_available = _PointerLikeTraits<_Pointer>::__low_bits_available;
+  static_assert(__pointer_low_bits_available >= __int_bit_count);
+
----------------
I would just inline it like this. I missed the `__pointer` prefix when I first read it and I thought this was a bug -- I think it's a bit confusing and doesn't save a lot of typing.


================
Comment at: libcxx/include/__utility/pointer_int_pair.h:115-117
+  static _LIBCPP_HIDE_FROM_ABI uintptr_t __to_uintptr(_PointerIntPair __ptr) { return __ptr.__value_; }
+
+  static _LIBCPP_HIDE_FROM_ABI _PointerIntPair __to_pointer(uintptr_t __ptr) {
----------------
Those could be constexpr, not that it wins us very much.


================
Comment at: libcxx/test/libcxx/utilities/pointer_int_pair/constructor_assert.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Let's name this `assert.constructor.pass.cpp` -- I've been trying to do this consistently.


================
Comment at: libcxx/test/libcxx/utilities/pointer_int_pair/pointer_int_pair.pass.cpp:13-16
+TEST_DIAGNOSTIC_PUSH
+TEST_CLANG_DIAGNOSTIC_IGNORED("-Wprivate-header")
+#include <__utility/pointer_int_pair.h>
+TEST_DIAGNOSTIC_POP
----------------
You could use `ADDITIONAL_COMPILE_FLAGS: -Wno-private-header` instead.


================
Comment at: libcxx/test/libcxx/utilities/pointer_int_pair/pointer_int_pair.pass.cpp:20
+
+int main(int, char**) {
+  {
----------------
Please test that you can `constinit` a pointer-int-pair.


================
Comment at: libcxx/test/libcxx/utilities/pointer_int_pair/pointer_int_pair.pass.cpp:22
+  {
+    std::__pointer_int_pair<int*, 1> pair = {};
+    assert(pair.__get_value() == 0);
----------------
Please test with an overaligned type and with more than 2 bits for the integer.


================
Comment at: libcxx/test/libcxx/utilities/pointer_int_pair/pointer_int_pair.pass.cpp:24
+    assert(pair.__get_value() == 0);
+    assert(pair.__get_ptr() == nullptr);
+  }
----------------
Please test with a non-`std::size_t` type, like bool and a few others.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140278