-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix memoryview compare #2357
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 memoryview compare #2357
Conversation
@qingshi163 I think you are one of the pro of memoryview. Could you review this PR please? |
vm/src/builtins/bytes.rs
Outdated
} else if other.isinstance(&vm.ctx.types.memoryview_type) { | ||
return Err(vm.new_type_error(format!( | ||
"'{}' not supported between instances of '{}' and '{}'", | ||
op.operator_token(), | ||
zelf.class().name, | ||
other.class().name | ||
))); | ||
} else { | ||
zelf.inner.cmp(other, op, vm) | ||
}) |
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.
The CPython's logic is very strange here, memoryview could do == or != here with bytes, but raise type error when doing <, >, <=, >=. and bytearray just accept everything.
This code does not match the cpython's logic for the following compare:
bytes (==,!=) memoryview
memoryview (<,<=,>,>=) bytearray
Thank you for your review. I'm not sure if I edited it as you said. |
I don't know any reason CPython has a back door for ord compare between memoryview and bytearray(not bytes). Should we follow? @coolreader18 |
Huh, that's really weird behavior. I think if there's no test cases that test for that (or even if there are, honestly) it's fine to ignore that. |
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.
Thank you for contributing!
No description provided.