<div dir="ltr">Doesn't it already do this for *all* types?  It's only using placement new, but the storage is stack-allocated.</div><br><div class="gmail_quote"><div dir="ltr">On Wed, May 17, 2017 at 8:57 AM Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</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">Yeah, I was about to suggest this.<br><div>We could specialize Optional for POD types so it doesn't spend time trying to new a buffer to store them :)</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, May 17, 2017 at 8:50 AM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br><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)?<div class="m_-4045782089302796406HOEnZb"><div class="m_-4045782089302796406h5"><br><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>
</div></div></blockquote></div><br></div>
</blockquote></div>