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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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

> 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.

Yeah, thanks for the summary!

Yeah, seems there's enough existing users of the (unsigned, bool) API that without making it invalid it'd be hard to restructure things to get in a better state, so probably a "named constructor"/factory function would be the best thing at the moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117114