[PATCH] D81580: [ADT] Add Bitfield utilities

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked 2 inline comments as done.
gchatelet added inline comments.


================
Comment at: llvm/unittests/ADT/BitFieldsTest.cpp:21
+  using Bool = Bitfield<bool, 0, 1>;
+  setField<Bool>(Storage, true);
+  EXPECT_EQ(Storage, 0b00000001);
----------------
gchatelet wrote:
> courbet wrote:
> > gchatelet wrote:
> > > serge-sans-paille wrote:
> > > > Would that make sense to use the following syntax instead of the setField method ?
> > > > 
> > > > ```
> > > > Bitfield<bool, 0, 1>(Storage) = true;
> > > > ```
> > > > 
> > > > This would also have the pro of not providing a relatively generic name `setField` in the llvm namespace.
> > > Are you suggesting the following API changes?
> > > ```
> > > // testField becomes cast to bool operator
> > > if(Bitfield<bool, 0, 1>::testField(Storage))
> > > =>
> > > if(Bitfield<bool, 0, 1>(Storage))
> > > 
> > > // getField becomes cast to UserType
> > > auto Value = Bitfield<bool, 0, 1>::getField(Storage);
> > > =>
> > > auto Value = Bitfield<bool, 0, 1>(Storage);
> > > 
> > > // setField becomes returning a UserType assignable object
> > > Bitfield<bool, 0, 1>::setField(Storage, Value);
> > > =>
> > > Bitfield<bool, 0, 1>(Storage) = Value;
> > > ```
> > > 
> > > Implementation wise this is more complex:
> > >  - we need to create one or two objects (one for `test`/`get`, and two for `set`),
> > >  - for the setter we need to return a type with a reference to Storage so we have to consider Storage lifetime,
> > >  - the returned object must not be copyable nor movable to prevent passing it to functions,
> > >  - cast operators may introduce type promotion so it's harder to understand what's going on,
> > >  - object may introduce indirection that prevent the compiler from generating clean code.
> > > 
> > > @courbet what do you think?
> > No strong opinion. Just make sure that the wrapper object for set is non movable and copyable for lifetime issues.
> I've created an alternative design in D81662.
> I couldn't get the `test` function as a cast operator because it would conflict with the cast to `UserType` when `UserType` is `bool`.
> It is not possible to use `enable_if` to disable the offending function because SFINAE only works for deduced template arguments, and nothing is to be deduced in this case (see this [[ https://stackoverflow.com/questions/50713692/why-enable-if-cannot-be-used-to-disable-this-declaration-here/50719848 | stackoverflow ]]).
> 
> Let me know what you think.
Sorry I mixed up the patches. the alternative design is here D82058


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81580