This is an archive of the discontinued LLVM Phabricator instance.

[ADT] Add BitVector::find_prev
ClosedPublic

Authored by zturner on May 4 2017, 3:45 PM.

Details

Summary
This almost completes the matrix of all possible find operations.

*EXISTING*
----------
find_first
find_first_unset
find_next
find_next_unset
find_last
find_last_unset

*NEW*
----
find_prev

*STILL MISSING*
---------------
find_prev_unset

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.May 4 2017, 3:45 PM
chandlerc added inline comments.May 4 2017, 4:29 PM
llvm/include/llvm/ADT/BitVector.h
220 ↗(On Diff #97888)

Unrelated change?

232 ↗(On Diff #97888)

Feel free to just commit typo fixes etc.

256 ↗(On Diff #97888)

I'd really like to move the doxygen comments away from repeating the name. Maybe in a follow-up? Definitely not in this patch.

257–258 ↗(On Diff #97888)

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?

llvm/unittests/ADT/BitVectorTest.cpp
236–258 ↗(On Diff #97888)

Feel free to land everything but find_prev right away with a separate patch?

zturner updated this revision to Diff 97899.May 4 2017, 5:35 PM

Removed all unrelated changes and committed them separately.

zturner updated this revision to Diff 97902.May 4 2017, 5:37 PM

Re-word the comment and parameter name to be more clear.

chandlerc accepted this revision.May 4 2017, 5:39 PM

LGTM, nice!

This revision is now accepted and ready to land.May 4 2017, 5:39 PM
This revision was automatically updated to reflect the committed changes.