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

[Jan Svoboda via Phabricator via llvm-commits]

jansvoboda11 added a comment.

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

> What're the other APIs like by comparison that @dexonsmith has in mind/is suggesting as alternatives? Maybe a quick summary of how similar operations are spelled with different APIs might be good to consider what way we should go.

I didn't find any data structures in LLVM/Clang with static member functions that take `std::initializer_list` and create new instance.

---

Both `std::bitset` and `boost::dynamic_bitset` have constructors that take `std::string`:

  std::bitset<8> x(std::string("01010101"))

Even if we used `StringRef` instead, I don't like that: it requires extra input validation.

---

Additionally, `std::bitset` supports `unsigned` constructor:

  std::bitset<8> x(0b01010101)

That conflicts with our existing `(unsigned s, bool t = false)` constructor though.

---

Another alternative would be a free factory function, similar to `make_pair`, `make_optional`, `makeIntrusiveRefCnt` etc.:

  auto x = makeBitVector({true, false, true, false, true})



---

I'm starting to think the most sensible approach would be either `llvm::BitVector::make({true, false, true})` or `llvm::makeBitVector({true, false, true})`.
While I was able to resolve the constructor ambiguity upstream, downstream projects that call `llvm::BitVector{unsigned, bool}` might get into trouble (i.e. hard to track down bugs) when they update to newer LLVM versions.



================
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);
----------------
dexonsmith wrote:
> 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?
No, this is a //default member initializer//, which can only be //brace// or //equals// initializer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117114