Skip to content

More efficient chomp for String #58192

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.

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jakobnissen
Copy link
Member

We can avoid all the computation of valid indices for this simple function, since it only strips CR (0xD) and LF (0xA), which are known to be single bytes. This is approximately four times faster.

We can avoid all the computation of valid indices for this simple function,
since it only strips CR (0xD) and LF (0xA), which are known to be single bytes.
This is approximately four times faster.
@jakobnissen jakobnissen added performance Must go faster strings "Strings!" labels Apr 22, 2025
@jakobnissen
Copy link
Member Author

CI failure is unrelated.

@jakobnissen jakobnissen added the merge me PR is reviewed. Merge when all tests are passing label Apr 23, 2025
@jakobnissen
Copy link
Member Author

Maybe it could have some @assume_effects - I believe it's total, but I'm not sure enough to annotate it with that. The compiler taints almost all the effects, unfortunately.

@jakobnissen jakobnissen removed the merge me PR is reviewed. Merge when all tests are passing label Apr 23, 2025
@jakobnissen
Copy link
Member Author

Added some effects, so removed the "merge me" tag since that's slightly risky.

@jakobnissen jakobnissen added the merge me PR is reviewed. Merge when all tests are passing label Apr 25, 2025
@DilumAluthge
Copy link
Member

@Keno @oscardssmith Is this ready to merge now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me PR is reviewed. Merge when all tests are passing performance Must go faster strings "Strings!"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants