Skip to content

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

Merged
merged 1 commit into from
Dec 14, 2020
Merged

fix memoryview compare #2357

merged 1 commit into from
Dec 14, 2020

Conversation

hyperbora
Copy link
Contributor

No description provided.

@youknowone
Copy link
Member

@qingshi163 I think you are one of the pro of memoryview. Could you review this PR please?

Comment on lines 534 to 546
} 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)
})
Copy link
Contributor

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

@hyperbora
Copy link
Contributor Author

Thank you for your review. I'm not sure if I edited it as you said.

@qingshi163
Copy link
Contributor

I don't know any reason CPython has a back door for ord compare between memoryview and bytearray(not bytes). Should we follow? @coolreader18
Otherwise this code fix at lease some of the case.

@coolreader18
Copy link
Member

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.

Copy link
Member

@coolreader18 coolreader18 left a 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!

@coolreader18 coolreader18 merged commit 9b76dc3 into RustPython:master Dec 14, 2020
@hyperbora hyperbora deleted the fix-memoryview-compare branch December 14, 2020 11:34
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.

4 participants