[PATCH] D81580: [ADT] Add Bitfield utilities

[Clement Courbet via Phabricator via llvm-commits]

courbet 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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81580