[PATCH] D81580: [ADT] Add Bitfield utilities

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked 4 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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81580