[PATCH] D82454: [ADT] Add Bitfield utilities - design #3

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.


================
Comment at: llvm/include/llvm/ADT/Bitfields.h:214
+  /// Describes an element of a Bitfield. This type is then used with
+  /// Bitfield::get \param T, the type of the field once in unpacked form,
+  /// \param Offset, the position of the first bit,
----------------
rriddle wrote:
> Did you intend for `\param T` to be on the next line?
Good catch. Documentation was half-done and `clang-format` messed up with the lines.


================
Comment at: llvm/include/llvm/ADT/Bitfields.h:219
+  template <typename T, unsigned Offset, unsigned Size,
+            T MaxValue = std::is_enum<T>::value
+                             ? T(0) // coupled with static_assert below
----------------
rriddle wrote:
> You could likely simplify this with SFINAE:
> 
> ```
> template <typename T, unsigned Offset, unsigned Size,
>         T MaxValue = std::numeric_limits<T>::max(),
>         bool IsEnum = std::is_enum<T>::value>
> struct Element {
> };
> 
> template <typename T, unsigned Offset, unsigned Size, T MaxValue>
> struct Element<T, Offset, Size, MaxValue, true>
>    : public Element<typename std::underlying_type<T>::type, Offset, Size, MaxValue> {
>   using Type = T;
> };
> ```
I've tried that but we need a few more tweaks to make it compile:
 - cast `MaxValue` to the underlying type in the enum specialization,
 - add a `using` directive to be able to access `IntegerType`at instantiation time.

With this, it's not that much simpler. Especially with the fact that you need to consider more code to understand the whole thing. WDYT?

```
  template <typename T, unsigned Offset, unsigned Size,
            T MaxValue = std::numeric_limits<T>::max(),
            bool IsEnum = std::is_enum<T>::value>
  struct Element {
  };

  template <typename T, unsigned Offset, unsigned Size, T MaxValue>
  struct Element<T, Offset, Size, MaxValue, true>
      : public Element<typename std::underlying_type<T>::type, Offset, Size,
                       static_cast<typename std::underlying_type<T>::type>(
                           MaxValue)> {
    using Type = T;

  private:
    using BaseT =
        Element<typename std::underlying_type<T>::type, Offset, Size,
                static_cast<typename std::underlying_type<T>::type>(MaxValue)>;
    static_assert(std::is_unsigned<typename BaseT::IntegerType>::value,
                  "Enum must be unsigned");
  };
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82454