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

Jan Svoboda via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jan 13 02:54:28 PST 2022


jansvoboda11 marked an inline comment as done.
jansvoboda11 added a comment.

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.

Though I'm okay with static factory function... I like `BitVector::make({true, false, true})`.

> I'm curious which current users of `std::vector<bool>` (if any) need this.

>From a quick search, I can see two users at this moment: `clang/unittests/Lex/HeaderSearchTest.cpp` and `flang/unittests/Runtime/Reduction.cpp`. Yes, those are "only" unit tests, but I can see myself choosing `std::vector<bool>` over `llvm::BitVector` just because it's much easier to match/compare in tests. The last thing I want to do in unit tests is to fight with the API of some container.

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

> Re: the ambiguity: We could pre-empt that in a separate commit by making the ctor "explicit" - honestly I think we shouldn't be using {} to call user-defined ctors that aren't representative of/"Feel like" conversions. (wouldn't mind that as a broad style statement for LLVM and/or in the form of "mark multi-arg ctors explicit unless they're intentionally converting/structural constructors".

Maybe I'm misunderstanding you, but the existing constructor already is marked as `explicit` and that doesn't make brace initialization invalid. Did you have something else in mind?



================
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:
> 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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117114



More information about the llvm-commits mailing list