Page MenuHomePhabricator

[ADT] Add Bitfield utilities - design #3
ClosedPublic

Authored by gchatelet on Jun 24 2020, 5:14 AM.

Details

Summary

Alternative design to D81580 with the following change in API

before: using Bool = Bitfield<bool, 0, 1>;
after:  using Bool = Bitfield::Element<bool, 0, 1>;

// Testing a field
before: if(Bool::testField(Storage))
after:  if(Bitfield::test<Bool>(Storage))

// Getting a field
before: auto Value = Bool::getField(Storage);
after:  auto Value = Bitfield::get<Bool>(Storage);

// Setting a field
before: Bool::setField(Storage, Value);
after:  Bitfield::set<Bool>(Storage, Value);

Diff Detail

Event Timeline

gchatelet created this revision.Jun 24 2020, 5:14 AM
gchatelet edited the summary of this revision. (Show Details)Jun 24 2020, 5:15 AM
courbet added inline comments.Jun 25 2020, 2:30 AM
llvm/include/llvm/ADT/Bitfields.h
78

details::Impl is a bit too generic and might collide. bitfields_detail ?

gchatelet updated this revision to Diff 273326.Jun 25 2020, 6:08 AM
gchatelet marked an inline comment as done.
  • Address comments
rriddle added inline comments.Jun 25 2020, 12:23 PM
llvm/include/llvm/ADT/Bitfields.h
205

typo: conditionnal

213

nit: Why the newline here?

215

Did you intend for \param T to be on the next line?

220

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;
};
gchatelet updated this revision to Diff 273635.Jun 26 2020, 2:45 AM
gchatelet marked 5 inline comments as done.
  • Address comments
llvm/include/llvm/ADT/Bitfields.h
215

Good catch. Documentation was half-done and clang-format messed up with the lines.

220

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 IntegerTypeat 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");
};

@serge-sans-paille let me know what you think of this version.
It's not object oriented but I find it pretty compelling.

@serge-sans-paille let me know what you think of this version.
It's not object oriented but I find it pretty compelling.

I'm fine with that API too, and it's implementation is simpler. I'll have a look to the implementation.

aprantl added inline comments.
llvm/include/llvm/ADT/Bitfields.h
16

consistent

94

Could you please use /// doxygen comments for (pretty much) all comments in this file?

rriddle marked an inline comment as done.Jun 26 2020, 12:28 PM
rriddle added inline comments.
llvm/include/llvm/ADT/Bitfields.h
220

SGTM

gchatelet updated this revision to Diff 274039.Jun 29 2020, 3:37 AM
gchatelet marked 3 inline comments as done.
  • Fix typo and doxygen comments
courbet accepted this revision.Jun 29 2020, 5:28 AM
courbet added inline comments.
llvm/include/llvm/ADT/Bitfields.h
21

ensure

193

semantics

This revision is now accepted and ready to land.Jun 29 2020, 5:28 AM
gchatelet marked 2 inline comments as done.Jun 29 2020, 6:01 AM
gchatelet edited the summary of this revision. (Show Details)Sep 7 2020, 9:07 AM