[PATCH] D81580: [ADT] Add Bitfield utilities

[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>;
+  setField<Bool>(Storage, true);
+  EXPECT_EQ(Storage, 0b00000001);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81580