<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Apr 14, 2016 at 5:11 PM, Matthias Braun <span dir="ltr"><<a href="mailto:matze@braunis.de" target="_blank">matze@braunis.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span class=""><blockquote type="cite"><div>On Apr 13, 2016, at 12:48 PM, David Blaikie via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:</div><br><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 13, 2016 at 11:50 AM, Matthias Braun via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">MatzeB added a comment.<br>
<span><br>
In <a href="http://reviews.llvm.org/D18578#400148" rel="noreferrer" target="_blank">http://reviews.llvm.org/D18578#400148</a>, @dblaikie wrote:<br>
<br>
> This seems to have strange behavior - returning the size/empty status of<br>
>  the BitVector for the BitSets operations is probably wrong, no? (the<br>
>  BitVector's length is the length of the field, not the number of set (1)<br>
>  bits in it)<br>
<br>
<br>
</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></blockquote><div><br></div><div>Ah, looks like you're right - my mistake.</div><div> </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><br></div><div>Seems expensive, though - that makes empty O(N)?<br></div></div></div></div></div></blockquote></span><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><span class=""><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>(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></span><div>To do this nicely I added a find_next_at() function to BitVector (see the updated patch).</div><span class=""><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>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></span><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></div></blockquote><div><br></div><div>I think that's probably worth doing, but happy to hear from others if they think it's not.<br><br>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.</div></div></div></div>