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

[serge via Phabricator via llvm-commits]

serge-sans-paille added a comment.

Additional thoughts: even though there wasn't a lot of answers to your initial thread on llvm-dev, I'd post a followup with a few examples of the two approaches to gather some feedback from the community. I personally like this one, obviously :-)



================
Comment at: llvm/unittests/ADT/BitFieldsTest.cpp:21
+  using Bool = Bitfield<bool, 0, 1>;
+  bitfield<Bool>(Storage) = true;
+  EXPECT_EQ(Storage, 0b00000001);
----------------
gchatelet wrote:
> 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 ?
@gchatelet : I really like the syntax eye candy. That looks very readable to me. thanks for going as far as implementing an alternate version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82058