-
Notifications
You must be signed in to change notification settings - Fork 12
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
Fix/out of bounds while decoding #37
Fix/out of bounds while decoding #37
Conversation
e13f8d5
to
7124ec5
Compare
7124ec5
to
faedc5e
Compare
I have gone back and forth on this a bit (as you can probably tell from the commits) - I'm happy with this version now, the performance hit seems very small. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to unwrap()
here: we return a Result
from this function so we can check for Err
values and pass them up the chain.
Perf regression on decodes is 10 - 12 %. I can live with that unless someone else has a better suggestion. |
Why aren't our tests running 🤔 |
@mattiZed Thanks, this looks OK to me. Could you also update the changelog (under |
Sure thing, done! |
This PR is a fix for #36.
I'd like to add that this issue was probably not caught earlier because the "broken_string" test is a bit misleading. While the polyline in that test is "broken" it still decodes fine, the decoded data is just meaningless. It would only trigger "should_panic" because of the test's assertion not holding. I have refactored another test with a similar issue.
I have also added a test with an input that actually causes the out-of-bounds error.
On my machine, there is a performance hit:
I can't tell if this is still acceptable.