[PATCH] D18578: Introduce BitSet: A BitVector based class behaving like std::set/DenseSet

[David Blaikie via llvm-commits]

On Thu, Apr 14, 2016 at 5:11 PM, Matthias Braun <matze at braunis.de> wrote:

>
> On Apr 13, 2016, at 12:48 PM, David Blaikie via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>
>
> On Wed, Apr 13, 2016 at 11:50 AM, Matthias Braun via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> MatzeB added a comment.
>>
>> In http://reviews.llvm.org/D18578#400148, @dblaikie wrote:
>>
>> > This seems to have strange behavior - returning the size/empty status of
>> >  the BitVector for the BitSets operations is probably wrong, no? (the
>> >  BitVector's length is the length of the field, not the number of set
>> (1)
>> >  bits in it)
>>
>>
>> BitSet::size() should return the number of elements in the set which
>> corresponds to the number of set bits in the bitvector which should be
>> BitVector::count().
>>
>
> Ah, looks like you're right - my mistake.
>
>
>> empty() checks BitSet::size() against 0 which should be equivalent to
>> BitVector::count() == 0.
>
>
> Seems expensive, though - that makes empty O(N)?
>
> Yes that makes it O(N). I'll change it BitVector::none() to speed it up. I
> don't believe adding a counter to make O(1) is a good deal here, I believe
> the empty()/size() functions are used rarely enough that adding and
> maintaining the counter will mostly slow down the average case and only
> gain us anything in exceptionally large inputs. We can also maintain a
> counter externally to the BitSet for places that really need it (if we ever
> find them).
>
>
> (you might be able to simplify 'begin()' by sharing some of the
> implementation of "advance" - usually filtering iterators like this would
> call "advance" in their ctor, to search through to find the first relevant
> element (advance, then, would probably be designed to do nothing if the
> current element is valid - so ++ would have to increment the iterator, then
> call advance, rather than just calling advance))
>
> To do this nicely I added a find_next_at() function to BitVector (see the
> updated patch).
>
>
> My other general question is: Do we need both these abstractions (BitSet
> and BitVector) or should we just change BitVector into BitSet & check all
> the callers/users?
>
> The big majority of users does inded only perform insert, erase and count
> like operations on the bitvectors. Changing this however is a big
> undertaking, we would also need to check that compilers correctly optimize
> if the result of insert() and erase() so we do not perform extra
> computations compared to BitVector::set()/BitVector::test()...
>

I think that's probably worth doing, but happy to hear from others if they
think it's not.

Certainly if no one really needs BitVector & they'd all be as happy with
BitSet, I think we probably don't want to keep them both around - just
going to be confusing.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160414/0bfae29e/attachment.html>