[PATCH] D23384: Use a byte array as an internal buffer of BitVector.

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: include/llvm/ADT/BitVector.h:45
@@ +44,3 @@
+  // Words are in the little endian order on all platforms.
+  MutableArrayRef<Word> Words;
+
----------------
zturner wrote:
> ruiu wrote:
> > zturner wrote:
> > > Since words are always little endian, why not make the type of Word be `support::ulittle32_t`
> > But in many cases you don't actually care about endianness. For example, for count(), only the number of 1s matters. You don't need to swap bytes. So I think using ulittle32_t is inefficient.
> But if you don't care about endianness, then it doesn't matter what you pass to popcnt either, so long as it contains the same number of 1s.  So you could write it like this:
> 
> ```
>   size_type count() const {
>     unsigned NumBits = 0;
>     for (unsigned i = 0; i < Bytes.size(); i += WORD_SIZE)
>       NumBits += countPopulation(*reinterpret_cast<uint32_t*>(&Bytes[i]));
>     return NumBits;
>   }
> ```
> 
> Maybe it's better to delete the `Words` array entirely and just do word-sized operations by casting the underlying byte vector.
I tried to remove `Words`, but the result did not look great. Casting to Word everywhere makes code longer and error-prone. As an example, current `count()` is this, which is much easier to understand than yours.

  size_type count() const {
    unsigned N = 0;
    for (Word W : Words)
      N += countPopulation(W);
    return N;
  }


https://reviews.llvm.org/D23384