-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Check operand types in bool or, and, xor to be PyInt #5461
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
Check operand types in bool or, and, xor to be PyInt #5461
Conversation
Flipping of parameter order to satisfy __r<op>__ dunder is done via macros
PyNumber methods are expected to type check both arguments. Dispatch is not done by inverting parameter order for __r<op>__ (example __ror__) when calls are handled via PyNumberMethods
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.
Good catch! Thank you.
Code changes looks perfect. Please check the comment part.
vm/src/vm/vm_ops.rs
Outdated
@@ -151,7 +151,7 @@ impl VirtualMachine { | |||
/// Calling scheme used for binary operations: | |||
/// | |||
/// Order operations are tried until either a valid result or error: | |||
/// b.rop(b,a)[*], a.op(a,b), b.rop(b,a) | |||
/// b.rop(a,b)[*], a.op(a,b), b.rop(a,b) |
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.
rop
is a reversed version of op
. So (b,a) is intended.
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.
Done. Had changed the comment looking at https://github.com/python/cpython/blob/main/Objects/abstract.c#L920
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.
Oh, thank you! we'd better to follow it later.
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.
oh, please also remove get_py_int
from bool.rs which is not used anymore
Thanks for the review! Made both changes, PTAL |
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.
👍
PyNumber methods are expected to type check both arguments.
Dispatch is not done by inverting parameter order for r (example ror) when calls are handled via PyNumberMethods
Other changes:
Fixes: #5460