Skip to content

Remove unnecessary slot wrapper addition - #2879

Merged
youknowone merged 2 commits into
RustPython:masterfrom
moreal:bugfix/unnecessary-slot-overwrite
Aug 14, 2021
Merged

Remove unnecessary slot wrapper addition#2879
youknowone merged 2 commits into
RustPython:masterfrom
moreal:bugfix/unnecessary-slot-overwrite

Conversation

@moreal

@moreal moreal commented Aug 14, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

This pull request resolves #2869.

Before (RustPython)

>>>>> class A: ...
>>>>> A.__new__
<bound method __new__ of <class '__main__.A'>>
>>>>> object.__new__
<bound method __new__ of <class 'object'>>
>>>>> A.__new__ == object.__new__
False

After (RustPython)

>>>>> class A: ...
>>>>> A.__new__
<bound method __new__ of <class 'object'>>
>>>>> object.__new__
<bound method __new__ of <class 'object'>>
>>>>> A.__new__ == object.__new__
True

@youknowone

youknowone commented Aug 14, 2021

Copy link
Copy Markdown
Member

Once this is successful, I think we also can remove the if statement in add_slot_wrappers

@moreal

moreal commented Aug 14, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Once this is successful, I think we also can remove the if statement in add_slot_wrappers

I understood the code you mean is below line. Sadly, when I remove the line, I met segmentation fault (core dumped) 😞

RustPython/vm/src/pyobject.rs

Line 392 in 61bae9a

if !ty.attributes.read().contains_key("__new__") {
See More

@youknowone

youknowone commented Aug 14, 2021

Copy link
Copy Markdown
Member

Yes. actually I expected the same thing for this patch itself also ;)

I don't know well the detail - so this is a rough guess. I think that line is added as a lazy workaround about double calling the method, mostly not by intension. Because you already removed the unnessessary duplicated call of the function. Maybe there will be another way to remove another duplicated call of it. (note: I am not meaning that work needs to be included in this patch)

@moreal
moreal marked this pull request as ready for review August 14, 2021 07:11
@youknowone
youknowone merged commit 9d99ea9 into RustPython:master Aug 14, 2021
@youknowone youknowone mentioned this pull request Aug 14, 2021
Merged
@youknowone youknowone added the z-ca-2021 Tag to track contrubution-academy 2021 label Oct 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Added undefined __new__ method to non-built-in types

2 participants

@moreal @youknowone