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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked 3 inline comments as done.
gchatelet 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.
----------------
courbet wrote:
> `The conversion from/to enum/bool are done in the public API outside of this template` where is that ?
Outdated comment, thx for noticing.
The documentation is not polished yet, I'd like to get in agreement on the API code first.
Keeping your comment not `Done` for now.


================
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;
----------------
courbet wrote:
> 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.
Yeah that's why I'm not fond of this version, the object semantic adds its own set of gotchas on top of the already complex functional code.


================
Comment at: llvm/unittests/ADT/BitFieldsTest.cpp:21
+  using Bool = Bitfield<bool, 0, 1>;
+  bitfield<Bool>(Storage) = true;
+  EXPECT_EQ(Storage, 0b00000001);
----------------
courbet wrote:
> 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`.
Yes unfortunately this is not possible as `Storage` type needs to be deduced somehow.
e.g.
```
template<typename UserType, unsigned Offset, unsigned Size, UserType MaxValue = ...>
struct Bitfield {
  Bitfield(StorageType &Storage) // where does StorageType comes from?
};
```

The type would need to be part of the template arguments.
e.g.
```
template<typename StorageType, typename UserType, unsigned Offset, unsigned Size, UserType MaxValue = ...>
struct Bitfield {
  Bitfield(StorageType &Storage) //OK
};
```
But I believe this is confusing to declare (e.g. `Bitfield<uint64_t, uint_8t, 0, 4>`) and would require a lot of editing in case the `StorageType` changes.

The function is the trick to deduce `StorageType` automatically.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82058