-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Implement Number Protocol for PyBool
#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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Number Protocol for PyBool
#4639
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 be None
.
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 seems int
operations are needed for bool
in CPython.
>>> (True + True) ** True
2
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.
Since PyBool
is a subclass of PyInt
and inherits its methods, so (True + True) ** True
should somehow work without the above overriding in as_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 filling PyNumberMethods
is not the way to do.
We can do that job only for builtin types written in Rust.
Making them None
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 call mro_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 call inner_and
#[pymethod(name = "__rand__")]
#[pymethod(magic)]
fn and(lhs: PyObjectRef, rhs: PyObjectRef, vm: &VirtualMachine) -> PyObjectRef {
Self::inner_and(&lhs, &rhs, vm)
}
fn inner_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, since get_py_int(&lhs).and(rhs, vm)
requires rhs
to be an PyObjectRef
as defined in int.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.
maybe rhs.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 would rhs.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. Adding TODO:
or FIXME:
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_bytes
True |
Closes #4638
In CPython,
bool
is a subclass ofint
, and itsas_number
method overridesand
,xor
andor
.In RustPython,
as, thePyBool
is not a subclass ofPyInt
as_number
"overriding" is done by including all other methods ofPyInt
.