[PATCH] D32885: [ADT] Add BitVector::find_prev

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: llvm/include/llvm/ADT/BitVector.h:220
     // Mask off previous bits.
-    Copy &= ~0UL << BitPos;
+    Copy &= maskTrailingZeros<BitWord>(BitPos);
 
----------------
Unrelated change?


================
Comment at: llvm/include/llvm/ADT/BitVector.h:232
 
-  /// find_next_unset - Returns the index of the next usnet bit following the
+  /// find_next_unset - Returns the index of the next unset bit following the
   /// "Prev" bit.  Returns -1 if all remaining bits are set.
----------------
Feel free to just commit typo fixes etc.


================
Comment at: llvm/include/llvm/ADT/BitVector.h:256
 
+  /// find_prev - Returns the index of the next set bit *preceding* the
+  /// \p Next bit.  Returns -1 if all previous bits are unset.
----------------
I'd really like to move the doxygen comments away from repeating the name. Maybe in a follow-up? Definitely not in this patch.


================
Comment at: llvm/include/llvm/ADT/BitVector.h:257-258
+  /// find_prev - Returns the index of the next set bit *preceding* the
+  /// \p Next bit.  Returns -1 if all previous bits are unset.
+  int find_prev(unsigned Next) {
+    if (Next == 0)
----------------
I find all the wording really confusing here.

"the next bit set *preceding* the `Next` bit" huh?

I think the argument would make much more sense named `Pos` or `Index` or something. Then you can talk about finding the *previous* bit set to match the name `find_prev`?


================
Comment at: llvm/unittests/ADT/BitVectorTest.cpp:236-258
+  // Also test with a vector that is small enough to fit in 1 word.
+  A.resize(20);
+  A.set(3);
+  A.set(4);
+  A.set(16);
+  EXPECT_EQ(16, A.find_last());
+  EXPECT_EQ(3, A.find_first());
----------------
Feel free to land everything but find_prev right away with a separate patch?


https://reviews.llvm.org/D32885