<div dir="ltr">I mean, if we're gonna go through the trouble of changing the return value or the arguments to *anything*, then we already have to deal with the potential consequences of fixing up every single call site.  So it's hard to avoid changing the interface without rocking the boat.  And if we're going to rock the boat anyway, might as well do it in the way that everyone prefers.  </div><br><div class="gmail_quote"><div dir="ltr">On Wed, May 17, 2017 at 10:04 AM Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, May 17, 2017 at 9:50 AM Zachary Turner via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">At the risk of asking a stupid question, how about returning Optional<unsigned> (in a subsequent patch of course)?<br></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>It seems a shame to have a separate bool rather than using the fact that '-1' (or 'npos' equivalently) is a sentinel value indicating failure.</div><div><br></div><div>But I can't imagine it matters a lot for these routines.</div><div><br></div><div>However, I don't think it does much to address the reasons I generally prefer signed arithmetic here. If your goal is just to not rock the boat and make everything unsigned, I don't guess I care much between `npos` and `Optional<unsigned>`.</div><div><br></div><div>-Chandler</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div dir="ltr">On Wed, May 17, 2017 at 8:36 AM Chandler Carruth via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">chandlerc accepted this revision.<br>
chandlerc added a comment.<br>
This revision is now accepted and ready to land.<br>
<br>
LGTM with the assert changes.<br>
<br>
<br>
<br>
================<br>
Comment at: llvm/include/llvm/ADT/BitVector.h:151<br>
+  /// [Begin, End).  Returns -1 if all bits in the range are unset.<br>
+  int find_first_in(unsigned Begin, unsigned End) const {<br>
+    if (Begin >= End)<br>
----------------<br>
zturner wrote:<br>
> chandlerc wrote:<br>
> > Given that we return an int, I would prefer that the arguments also be int (and the intermediate variables within).<br>
> The only reason we return an int is because we have to use a sentinel value to indicate "no bits found".  The methods that these originated from `find_next`, `find_prev`, etc have always taken `unsigned` arguments even before I started tinkering on `BitVector`.  Do you still think it's a good idea to change all of them?<br>
I mean, my view is that we're doing a lot of arithmetic on these things including potentially subtraction. So generally, unless there is a problem with using a signed integer, I would prefer that.<br>
<br>
If we return `int`, then we've given up on having more than `2^31 - 1` bits in one of these containers.<br>
<br>
That said, a bitvector is one of the few datastructures where it seems entirely conceivable that we'll actually end up with 500mb vector of bits and an `int` index won't work. But an `unsigned` won't either, so I suspect that isn't the deciding factor and if we care we need a 64-bit type.<br>
<br>
<br>
So I guess my feeling is that we should do one of two things here:<br>
a) Switch the return types to an unsigned type and use something like `npos`, or<br>
b) Switch the argument types to a signed type and continue to use `-1`.<br>
<br>
(Whether we use 32-bits or 64-bits should be orthogonal as I see no reason why for a *bit* count 2gb vs 4gb would be "enough".)<br>
<br>
However, looking at the code, I agree that the patch as-is remains consistent, and you shouldn't do either (a) or (b) in *this* patch. But I'd somewhat like to consider doing one of them in a subsequent patch.<br>
<br>
My personal, strong preference is to use (b) because I find asserts easier to write for signed types in the face of arithmetic and we have strong usage of UBSan to ensure we don't develop bugs here. I also have a somewhat powerful dislike of `npos`. But I think either (a) or (b) would be an improvement. Anyways, sorry for the digression.<br>
<br>
<br>
================<br>
Comment at: llvm/include/llvm/ADT/BitVector.h:152-153<br>
+  int find_first_in(unsigned Begin, unsigned End) const {<br>
+    if (Begin >= End)<br>
       return -1;<br>
<br>
----------------<br>
zturner wrote:<br>
> chandlerc wrote:<br>
> > Why isn't this an assert?<br>
> I suppose I should assert if `Begin > End`, but `Begin == End` should probably still just return -1 I think.<br>
Ah, yes, that makes sense.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D33104" rel="noreferrer" target="_blank">https://reviews.llvm.org/D33104</a><br>
<br>
<br>
<br>
</blockquote></div></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>
</blockquote></div>