[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/include/llvm/ADT/Bitfields.h:90
+
+  static constexpr unsigned TypeBits = sizeof(Unsigned) * CHAR_BIT;
+  static_assert(TypeBits >= Bits, "n-bit must fit in T");
----------------
kpdev42 wrote:
> Maybe it is worth to use `std::numeric_limits<T>::digits` here?
> Like this:
> ```
> static constexpr auto TypeBits = std::numeric_limits<Unsigned>::digits;
> ```
> then it will be possible to remove `#include <climits> // CHAR_BIT`
Thx for the suggestion. Unfortunately I also use `CHAR_BIT` elsewhere in the file and [cppreference.com](https://en.cppreference.com/w/cpp/types/numeric_limits/digits) says:
> For integer types, this is the number of bits not counting the sign bit and the padding bits (if any)

 - For `Bitfield::TypeBits` the type may be signed,
 - For `Bitfield::Impl::StorageBits` I need the full storage space so padding bit must be accounted for.

With this in mind it seems to me that it's still beneficial to keep the `climits` include.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81580