Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Simplify inv memchr + add more safety comments #196

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Simplify inv memchr + add more safety comments #196

wants to merge 1 commit into from

Conversation

anforowicz
Copy link
Contributor

I hope this helps?

@anforowicz
Simplify safety checks in inv_memchr, LOOP_SIZE-stepping loop.
8f3d10a 
Before this commit the safety of `end_ptr.sub(loop_size)` depended on a
somewhat remote and indirect reasoning that `haystack.len() >=
LOOP_SIZE` if `loop_size == LOOP_SIZE` (that reasoning was based on
having `let loop_size = cmp::min(LOOP_SIZE, haystack.len())`).

After this commit, the safety checks are done in an `if` statement right
above where `end_ptr.sub(LOOP_SIZE)` happens.  This simplification also
allowed removing the `loop_size` binding/variable.
@anforowicz anforowicz force-pushed the simplify-inv-memchr branch 2 times, most recently from 3d3d11b to 8f3d10a Compare November 14, 2024 21:20
@anforowicz
Copy link
Contributor Author

The force-push above did 2 things:

  • It rebased the "Simplify inv memchr" commit on top of the most recent upstream commits
  • It temporarily dropped the follow-up cleanup that split big unsafe blocks into more granular ones (it's probably best for me to hold onto this for now, to simplify reviewing and merging the current PR; I can submit this as a separate PR later on if this one lands)

PTAL?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@anforowicz