[PATCH] D33104: [BitVector] Implement find_[first/last]_[set/unset]_in

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/include/llvm/ADT/BitVector.h:151
+  /// [Begin, End).  Returns -1 if all bits in the range are unset.
+  int find_first_in(unsigned Begin, unsigned End) const {
+    if (Begin >= End)
----------------
chandlerc wrote:
> Given that we return an int, I would prefer that the arguments also be int (and the intermediate variables within).
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?  


================
Comment at: llvm/include/llvm/ADT/BitVector.h:152-153
+  int find_first_in(unsigned Begin, unsigned End) const {
+    if (Begin >= End)
       return -1;
 
----------------
chandlerc wrote:
> Why isn't this an assert?
I suppose I should assert if `Begin > End`, but `Begin == End` should probably still just return -1 I think.


https://reviews.llvm.org/D33104