[PATCH] D81580: [ADT] Add Bitfield utilities
Guillaume Chatelet via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 17 01:02:42 PDT 2020
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
More information about the llvm-commits
mailing list