[PATCH] D117114: [llvm][ADT] Implement `BitVector(std::initializer_list)`

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

In D117114#3240161 <https://reviews.llvm.org/D117114#3240161>, @jansvoboda11 wrote:

> In D117114#3238494 <https://reviews.llvm.org/D117114#3238494>, @dexonsmith wrote:
>
>> It might be awkward to add this new ambiguity. I wonder if we really want to copy that from `std::vector<bool>`, which doesn't have a great API for bitsets anyway. @dblaikie, any thoughts?
>
> Since people are used to such constructors from the standard library and ADT as well (e.g. `SmallVector`), I think it would be good to maintain consistency.

My general point is that for a bitset data structure, std::bitset and boost::dynamic_bitset are more compelling data structures for matching APIs than is `std::vector<bool>`. (BTW, I agree supporting unit tests well is important; wasn't intending to argue otherwise.)

In D117114#3239411 <https://reviews.llvm.org/D117114#3239411>, @dblaikie wrote:

> In D117114#3238494 <https://reviews.llvm.org/D117114#3238494>, @dexonsmith wrote:
>
>> 
>
> What aspect of "not greatness" API design do you mean - having both the `(unsigned bool)` and init list ctors at the same time? I think with the former being marked as explicit it's /probably/ OK to me? () does "interesting" things, {} represents the value in some sense.

Yeah, it's the ambiguity on `{}` that seemed awkward to me; and maybe it's not that bad after all. As long as the interesting behaviour on `()` is not going to change the meaning of existing code then probably it's okay as-is.



================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:419-422
+  SmallBitVector TypeIdxsCovered = SmallBitVector(
+      MCOI::OPERAND_LAST_GENERIC - MCOI::OPERAND_FIRST_GENERIC + 2);
+  SmallBitVector ImmIdxsCovered = SmallBitVector(
+      MCOI::OPERAND_LAST_GENERIC_IMM - MCOI::OPERAND_FIRST_GENERIC_IMM + 2);
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > I assume this (and other changes here) are because there's a new constructor ambiguity?
> > 
> That's correct. Without these changes, the new constructor would be called instead, implicitly converting the `unsigned` argument to `bool`.
Does this simpler change also work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117114