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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked an inline comment as done.
gchatelet added inline comments.


================
Comment at: llvm/unittests/ADT/BitFieldsTest.cpp:21
+  using Bool = Bitfield<bool, 0, 1>;
+  bitfield<Bool>(Storage) = true;
+  EXPECT_EQ(Storage, 0b00000001);
----------------
courbet wrote:
> gchatelet wrote:
> > 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.
> That makes sense. Then maybe at least make the function static and part of the class ?
> 
> ```
> using Bool = Bitfield<bool, 0, 1>;
> Bool::fieldOf(Storage) = true;
> ```
> 
> That's a midpoint between the current proposal:
> 
> ```
> using Bool = Bitfield<bool, 0, 1>;
> bitfield<Bool>(Storage) = true;
> ```
> 
> And the previous one:
> 
> ```
> using Bool = Bitfield<bool, 0, 1>;
> Bool::setField(Storage, true);
> ```
> 
@jfb would you have some time to give your thoughts on which approach is best? (I've added you as a reviewer since you seemed interested in abstractions when I introduced `Align`.)
@serge-sans-paille what do you think of the API you suggested ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82058