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.

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

Inline peek_ahead to fix 186 and remove iter::peek_ahead as it is now unused. #188

Closed
wants to merge 1 commit into from

Conversation

hkBst
Copy link
Contributor

@hkBst hkBst commented Oct 22, 2024

Fixes #186

@hkBst
Copy link
Contributor Author

hkBst commented Oct 23, 2024

Actually, is there really any reason for this complicated dance? Why not just match on "POST "? It seems that would probably do fewer bounds checks as well. Or if it does one for GET and then another one for POST, maybe it would work to match on bytes.len()?

@seanmonstar
Copy link
Owner

I can say that method was very carefully adjusted to improve performance significantly.

@hkBst
Copy link
Contributor Author

hkBst commented Oct 30, 2024

Right, well this patch just inlines what the compiler was asked to inline, possibly a slightly more efficient version, so there should be no performance regression.

Is there anything you dislike about this approach?

@seanmonstar
Copy link
Owner

Hm, could we just make peek_ahead unsafe and revert back to using cursor.add(n) once, adding a comment that the caller must be sure n is within the range? I think that'd keep the one place it's called a little cleaner.

@hkBst
Copy link
Contributor Author

hkBst commented Nov 19, 2024

Alright, after some thought I agree with you that this is not very nice, since we are breaking out of the Bytes abstraction, so I am closing this in favor of pull #191.

@hkBst hkBst closed this Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bytes::peek_ahead is unsound
2 participants