<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Apr 13, 2016, at 12:48 PM, David Blaikie via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Wed, Apr 13, 2016 at 11:50 AM, Matthias Braun via llvm-commits <span dir="ltr" class=""><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">MatzeB added a comment.<br class="">
<span class=""><br class="">
In <a href="http://reviews.llvm.org/D18578#400148" rel="noreferrer" target="_blank" class="">http://reviews.llvm.org/D18578#400148</a>, @dblaikie wrote:<br class="">
<br class="">
> This seems to have strange behavior - returning the size/empty status of<br class="">
> the BitVector for the BitSets operations is probably wrong, no? (the<br class="">
> BitVector's length is the length of the field, not the number of set (1)<br class="">
> bits in it)<br class="">
<br class="">
<br class="">
</span>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().<br class=""></blockquote><div class=""><br class=""></div><div class="">Ah, looks like you're right - my mistake.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">empty() checks BitSet::size() against 0 which should be equivalent to BitVector::count() == 0. </blockquote><div class=""><br class=""></div><div class="">Seems expensive, though - that makes empty O(N)?<br class=""></div></div></div></div></div></blockquote><div>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).</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class="">(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))</div></div></div></div></div></blockquote><div>To do this nicely I added a find_next_at() function to BitVector (see the updated patch).</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class="">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?</div></div></div></div></div></blockquote><div>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()...</div><div><br class=""></div><div>- Matthias</div><div><br class=""></div></div></body></html>