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

[Matthias Braun via llvm-commits]

> 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 <mailto:llvm-commits at lists.llvm.org>> wrote:
> MatzeB added a comment.
> 
> In http://reviews.llvm.org/D18578#400148 <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()...

- Matthias

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160414/af6a0256/attachment.html>