make magic method new to follow cpython way#3057
Conversation
| impl PyBaseObject { | ||
| #[pyclassmethod(magic)] | ||
| fn new(cls: PyTypeRef, args: FuncArgs, vm: &VirtualMachine) -> PyResult { | ||
| let (subtype, args): (PyTypeRef, FuncArgs) = args.bind(vm)?; |
There was a problem hiding this comment.
SlotConstructor will be preferred for newly added __new__ with binding.
There was a problem hiding this comment.
I thought there may be collision with object.tp_new. is it okay to put both?
There was a problem hiding this comment.
oh! I am sorry. you are right. this is not tp_new but just __new__.
eldpswp99
force-pushed
the
make-magic-method-new-to-follow-cpython
branch
from
7202125 to
b3e1c84
Compare
youknowone
force-pushed
the
make-magic-method-new-to-follow-cpython
branch
2 times, most recently
from
824e259 to
76370b2
Compare
youknowone
force-pushed
the
make-magic-method-new-to-follow-cpython
branch
5 times, most recently
from
0a70569 to
d436231
Compare
youknowone
force-pushed
the
make-magic-method-new-to-follow-cpython
branch
3 times, most recently
from
45a6976 to
e82989d
Compare
| exceptions, | ||
| int_cache_pool, | ||
| string_cache, | ||
| tp_new_wrapper, |
There was a problem hiding this comment.
we need this field yet in this implementation
youknowone
force-pushed
the
make-magic-method-new-to-follow-cpython
branch
from
e82989d to
00b33f8
Compare
|
|
|
@coolreader18 @DimitrisJim could you review this PR? |
| #[pyimpl(with(SlotGetattro, SlotSetattro, Callable), flags(BASETYPE))] | ||
| impl PyType { | ||
| // bound method for every type | ||
| pub(crate) fn __new__(zelf: PyRef<Self>, args: FuncArgs, vm: &VirtualMachine) -> PyResult { |
There was a problem hiding this comment.
__new__ is sort of confusing to me, I'd prefer if this kept tp_new_wrapper so we can easily see which function this maps to in CPython.
There was a problem hiding this comment.
This is a body of bound method __new__ just like mro. The only difference of them is that __new__ is a bound method for every type including type but mro is an unbound method for type and bound as boundmethod for other types.
I think we'd better to keep this name as same as other methods when it works like other methods.
There was a problem hiding this comment.
I couldn't clarify this concepts well at this time, but now things seem well aligned.
new_wrapper is now meaning python-side wrapper of __new__. And this is a wrapper for type slot new.
|
this looks just fine to me but I've not really looked into it much so I'll leave the approval for Noa. |
coolreader18
left a comment
coolreader18
left a comment
There was a problem hiding this comment.
LGTM, I think 😅
This whole part of the interpreter is pretty complex and I don't really have an intuition for how it works, so given that this seems simpler and still works it's got my approval 👍
4 participants
this issue was first mentioned in #2974
This makes tp_new to be called by
object.__new__for example, with
tuple.__new__before:
<bound method __new__ of <class 'tuple'>>after:
<bound method object.__new__ of <class 'tuple'>>well, i have problem with controlling the new with pysuper, i remain this as draft now
I would be thankful for any advice!