- Notifications
You must be signed in to change notification settings - Fork1.3k
Implement Number Protocol forPyBool#4639
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Implement Number Protocol forPyBool#4639
Uh oh!
There was an error while loading.Please reload this page.
Conversation
In RustPython PyBool is a subclass of PyInt |
| add: int_method!(add), | ||
| subtract: int_method!(subtract), | ||
| multiply: int_method!(multiply), | ||
| remainder: int_method!(remainder), | ||
| divmod: int_method!(divmod), | ||
| power: int_method!(power), | ||
| negative: int_method!(negative), | ||
| positive: int_method!(positive), | ||
| absolute: int_method!(absolute), | ||
| boolean: int_method!(boolean), | ||
| invert: int_method!(invert), | ||
| lshift: int_method!(lshift), | ||
| rshift: int_method!(rshift), |
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.
Not sure yet, but I think those fields in bool have to beNone.
In my opinion, If that's not working with None, any inherited object of numbers can make similar problem.
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.
It seemsint operations are needed forbool in CPython.
>>> (True + True) ** True2There 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.
SincePyBool is a subclass ofPyInt and inherits its methods, so(True + True) ** True should somehow work without the above overriding inas_number I think.
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 am sorry to make confusion. I agree bool need int operations.
But I'd like to say manually fillingPyNumberMethods is not the way to do.
We can do that job only for builtin types written in Rust.
Making themNone and finding a way to make it work will be helpful to solve the problem in general.
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.
Did not test but I think PyBool is working with int number methods via mro search in PyNumber::find_methods().
But Maybe we can override it in PyBool to have some optimize
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, it wasn't. Because that is a static variable. We have no way to edit 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.
@qingshi163 do you have any suggestion?
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 think we can inherit slots when initializing the subclass
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.
slots will be inherit if subclass does not override it.
when there is a change of slots on the type, cpython recusively update slots for all the subclasses.
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.
slots will be inherit if subclass does not override it.
It is true in cpython but not here yet. That's the reason we always callmro_find_map() to find slots.
| and: atomic_func!(|number, other, vm| { | ||
| PyBool::and(number.obj.to_owned(), other.to_owned(), vm).to_pyresult(vm) | ||
| }), | ||
| xor: atomic_func!(|number, other, vm| { | ||
| PyBool::xor(number.obj.to_owned(), other.to_owned(), vm).to_pyresult(vm) | ||
| }), | ||
| or: atomic_func!(|number, other, vm| { | ||
| PyBool::or(number.obj.to_owned(), other.to_owned(), vm).to_pyresult(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.
Here is more room to optimize because and, xor, or actually doesn't require to increase reference count.
But it looks working fine.
To fully optimize it,PyBool::and can be changed like this and AsNumber can callinner_and
#[pymethod(name ="__rand__")]#[pymethod(magic)]fnand(lhs:PyObjectRef,rhs:PyObjectRef,vm:&VirtualMachine) ->PyObjectRef{Self::inner_and(&lhs,&rhs, vm)}fninner_and(lhs:&PyObject,rhs:&PyObject,vm:&VirtualMachine) ->PyObjectRef{if lhs.fast_isinstance(vm.ctx.types.bool_type) && rhs.fast_isinstance(vm.ctx.types.bool_type){let lhs =get_value(&lhs);let rhs =get_value(&rhs);(lhs && rhs).to_pyobject(vm)}else{get_py_int(&lhs).and(rhs, vm).to_pyobject(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.
This approach seems not working directly, sinceget_py_int(&lhs).and(rhs, vm) requiresrhs to be anPyObjectRef as defined inint.rs.
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.
mayberhs.to_owned() fornow. we can optimize int.and also in same way 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.
I'm not sure but wouldrhs.to_owned() be an extra burden? If it's ok (for now), I will adopt the above practice.
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.
Yes,rhs.to_owned() is extra cost, and still same as previous one. AddingTODO: orFIXME: can be helpful.
It is on the way to go to the right design.
The inheritance is set like this: |
I'm not sure, but does that mean |
Yes, >>>>>int.to_bytes==bool.to_bytesTrue |
Uh oh!
There was an error while loading.Please reload this page.
Closes#4638
In CPython,
boolis a subclass ofint, and itsas_numbermethod overridesand,xorandor.In RustPython,
as, thePyBoolis not a subclass ofPyIntas_number"overriding" is done by including all other methods ofPyInt.