Skip to content

Implement Number protocol for PyNone#3880

Merged
youknowone merged 3 commits into
RustPython:mainfrom
hy-kiera:none-pynumber
Jul 16, 2022
Merged

Implement Number protocol for PyNone#3880
youknowone merged 3 commits into
RustPython:mainfrom
hy-kiera:none-pynumber

Conversation

@hy-kiera

Copy link
Copy Markdown
Contributor

Implemented based on #3838

@hy-kiera hy-kiera marked this pull request as draft July 14, 2022 04:01
@hy-kiera hy-kiera marked this pull request as ready for review July 14, 2022 04:03
@youknowone youknowone requested a review from qingshi163 July 14, 2022 04:04
Comment thread vm/src/builtins/singletons.rs Outdated

impl AsNumber for PyNone {
const AS_NUMBER: PyNumberMethods = PyNumberMethods {
boolean: Some(|number, _vm| Ok(self::number_downcast(number).value.is_zero()))

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non_bool(), which is of none_as_number, returns 0 in cpython Objects/object.c

static int
none_bool(PyObject *v)
{
    return 0;
}

Copy link
Copy Markdown
Contributor

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 our bool returns false as you know, we just need to return what bool returns exactly.

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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?

Copy link
Copy Markdown
Contributor

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 😊

@youknowone youknowone added the z-ca-2022 Tag to track contrubution-academy 2022 label Jul 14, 2022
Comment thread vm/src/builtins/singletons.rs Outdated
}

#[pyimpl(with(Constructor))]
#[pyimpl(flags(BASETYPE), with(Constructor, AsNumber))]

@Snowapril Snowapril Jul 14, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[pyimpl(flags(BASETYPE), with(Constructor, AsNumber))]
#[pyimpl(with(Constructor, AsNumber))]

It seems PyNone does not have BASETYPE flags in cpython. Is there any reason for adding it?

@Snowapril Snowapril Jul 14, 2022

Copy link
Copy Markdown
Contributor

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_BASETYPE
1024
>>>>> 

CPython 3.10

>>> Py_TPFLAGS_BASETYPE = 1 << 10
>>> type(None).__flags__ & Py_TPFLAGS_BASETYPE
0

Comment thread vm/src/builtins/singletons.rs Outdated
}

#[pyimpl(with(Constructor))]
#[pyimpl(flags(BASETYPE), with(Constructor, AsNumber))]

Copy link
Copy Markdown
Member

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 think None is a BASETYPE.

@Snowapril

Copy link
Copy Markdown
Contributor

Please run cargo fmt --all 😊

@hy-kiera

Copy link
Copy Markdown
Contributor Author

Please run cargo fmt --all 😊

Okay. I'll run it.

Comment thread vm/src/builtins/singletons.rs Outdated

impl AsNumber for PyNone {
const AS_NUMBER: PyNumberMethods = PyNumberMethods {
boolean: Some(|number, _vm| Ok(Self::number_downcast(number).bool())),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Copy Markdown
Contributor Author

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 as repr() and bool() used be for?

@Snowapril Snowapril Jul 15, 2022

Copy link
Copy Markdown
Contributor

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'

Copy link
Copy Markdown
Member

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 functions bool(None) and repr(None)

Comment thread vm/src/builtins/singletons.rs Outdated
Comment on lines +3 to +4
class::PyClassImpl, convert::ToPyObject, protocol::PyNumberMethods, types::AsNumber,
types::Constructor, Context, Py, PyObjectRef, PyPayload, PyResult, VirtualMachine,

Copy link
Copy Markdown
Member

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}

@qingshi163 qingshi163 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great

@youknowone youknowone merged commit 10327b5 into RustPython:main Jul 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

z-ca-2022 Tag to track contrubution-academy 2022

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants