[PATCH] D82058: [ADT] Add Bitfield utilities - alternative design

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: llvm/include/llvm/ADT/Bitfields.h:143
+/// `BitfieldProperties` stores important properties about the Bitfield.
+/// It only deals with proper integer types. The conversion from/to enum/bool
+/// are done in the public API outside of this template.
----------------
`The conversion from/to enum/bool are done in the public API outside of this template` where is that ?


================
Comment at: llvm/include/llvm/ADT/Bitfields.h:196
+    Impl(const Impl &) = delete;
+    Impl(Impl &&) = default; // needed to return the value
+    Impl &operator=(const Impl &) = delete;
----------------
I'm actually a bit frightened by this, even with the guaranteed elision in c++, this is still dangerous: https://godbolt.org/z/t-Gzi7

The previous `Bitfield::setField()` approach has the same issue, but at least we're returning a reference and not a value, so the issue is more visible.


================
Comment at: llvm/include/llvm/ADT/Bitfields.h:245
+                           : std::numeric_limits<T>::max()>
+struct Bitfield {
+  using Type = T;
----------------
This is only a type, make ctor private to make it clear that it's not meant to be instantiated ?


================
Comment at: llvm/include/llvm/ADT/Bitfields.h:265
+/// Returns whether the two bitfields share common bits.
+template <typename A, typename B> static constexpr bool isOverlapping() {
+  using PA = typename A::Properties;
----------------
This should not be static. It can be `inline`, but this is already implied by the template.


================
Comment at: llvm/unittests/ADT/BitFieldsTest.cpp:21
+  using Bool = Bitfield<bool, 0, 1>;
+  bitfield<Bool>(Storage) = true;
+  EXPECT_EQ(Storage, 0b00000001);
----------------
I think @serge-sans-paille was suggesting to go one step further : do away with the `bitfield` function, and replace it with the ctor directly: `Bool(Storage) = true`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82058