- Notifications
You must be signed in to change notification settings - Fork1.3k
Implement Number protocol for PyNone#3880
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
vm/src/builtins/singletons.rs Outdated
| @@ -52,6 +54,13 @@ impl PyNone { | |||
| } | |||
| } | |||
| impl AsNumber for PyNone { | |||
| const AS_NUMBER: PyNumberMethods = PyNumberMethods { | |||
| boolean: Some(|number, _vm| Ok(self::number_downcast(number).value.is_zero())) | |||
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.
is_zero? the value looking swapped
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.
non_bool(), which is ofnone_as_number, returns 0 in cpythonObjects/object.c
static intnone_bool(PyObject *v){ return 0;}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.
return zero means false in this case. As ourbool returns false as you know, we just need to return whatbool returns exactly.
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, sorry. I was confused with bool.
as you said, it returns false(0). But this code is not simply returning false. make this line to do return false same to the code you shared.
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.
Hi. I fixed my code. Could you please review the new commit?
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.
Looks good to me now 😊
vm/src/builtins/singletons.rs Outdated
| @@ -39,7 +41,7 @@ impl Constructor for PyNone { | |||
| } | |||
| } | |||
| #[pyimpl(with(Constructor))] | |||
| #[pyimpl(flags(BASETYPE),with(Constructor, AsNumber))] | |||
SnowaprilJul 14, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
| #[pyimpl(flags(BASETYPE),with(Constructor,AsNumber))] | |
| #[pyimpl(with(Constructor,AsNumber))] |
It seemsPyNone does not haveBASETYPE flags in cpython. Is there any reason for adding it?
SnowaprilJul 14, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
RustPython (by current change)
>>>>>Py_TPFLAGS_BASETYPE=1<<10>>>>>type(None).__flags__&Py_TPFLAGS_BASETYPE1024>>>>>
CPython 3.10
>>>Py_TPFLAGS_BASETYPE=1<<10>>>type(None).__flags__&Py_TPFLAGS_BASETYPE0
vm/src/builtins/singletons.rs Outdated
| @@ -39,7 +41,7 @@ impl Constructor for PyNone { | |||
| } | |||
| } | |||
| #[pyimpl(with(Constructor))] | |||
| #[pyimpl(flags(BASETYPE),with(Constructor, AsNumber))] | |||
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.
Is this flag required for PyNone or AsNumber? I don't thinkNone is a BASETYPE.
Please run |
Okay. I'll run it. |
vm/src/builtins/singletons.rs Outdated
| @@ -52,6 +52,13 @@ impl PyNone { | |||
| } | |||
| } | |||
| impl AsNumber for PyNone { | |||
| const AS_NUMBER: PyNumberMethods = PyNumberMethods { | |||
| boolean: Some(|number, _vm| Ok(Self::number_downcast(number).bool())), | |||
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.
| boolean:Some(|number, _vm|Ok(Self::number_downcast(number).bool())), | |
| boolean:Some(|number, _vm|Ok(false)), |
None is singleton. I am pretty sure you can skip actual object value.
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 idea. BTW, what is the object's functions such asrepr() andbool() used be for?
SnowaprilJul 15, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
They are exposed as python method by#[pymethod] macro attached. You can call them below way.
>>>>>None.__bool__()False>>>>>None.__repr__()'None'
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.
And those functions@Snowapril referred are used by facade functionsbool(None) andrepr(None)
vm/src/builtins/singletons.rs Outdated
| class::PyClassImpl, convert::ToPyObject,protocol::PyNumberMethods, types::AsNumber, | ||
| types::Constructor, Context, Py, PyObjectRef,PyPayload, PyResult, VirtualMachine, |
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.
you can also group type traits
types::{AsNumber,Constructor}
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.
Looks great
Implemented based on#3838