-
Notifications
You must be signed in to change notification settings - Fork 308
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
Rewrite multibyte_length with slice patterns #1580
base: master
Are you sure you want to change the base?
Conversation
rust_src/src/multibyte.rs
Outdated
// The bits a byte "starts" with are the most significant ones in these comments | ||
match slice { | ||
// true if a starts with 0 | ||
&[a] if a & 0x80 == 0 => Some(1), |
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.
Don't all of the match patterns have to have wildcards at the end? I think this one should be &[a, ..]
, for example.
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.
That's true, the original code checks for failure conditions rather than success conditions and that has tripped me up a couple of times.
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.
Awesome, and +1 for adding tests!
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.
Nice!
&[a, b, c, d, e, ..] | ||
if a == 0xF8 && b & 0xF0 == 0x80 && (c & 0xC0) | (d & 0xC0) | (e & 0xC0) == 0x80 => | ||
{ | ||
Some(5) | ||
} |
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.
I do hate to just burst in like this, but you've changed the behavior here. This code won't return the same result for
a=0xF8, b=0x80. c=0x00, d=0x00, e=0xC0
, your code will return length 5, and the old code length None.
I haven't verified this so feel free to ignore me, but I do have a hunch!
Happy coding! :D
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 I think you're right, the old code required the b to be a continuation byte at an earlier point in the code, but since constraints don't cascade in this version I'll have to add the extra check. Thanks for catching this!
Since advanced slice patterns have been stablized, I've been meaning to rewrite this function using them, because I have a very hard time keeping its logic in its current form straight in my head. I also added comments clarifying exactly which bits are being checked for what.
I seem to have made some kind of logic error though, because the build fails, and I can't figure out where the error is.