Skip to content

Method overhaul with static PyMethodDef #4873

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 5, 2023
Merged

Method overhaul with static PyMethodDef #4873

merged 6 commits into from
May 5, 2023

Conversation

youknowone
Copy link
Member

@youknowone youknowone commented Apr 12, 2023 •
edited
Loading

I found #4686 is related to multiple design caveats. I originally tried to edit small enough to support the original PR, but found this way will reduce total cost of effort about them.

closes #4686 by including all its changes.

@youknowone youknowone marked this pull request as draft April 12, 2023 18:10
@youknowone youknowone force-pushed the method branch 9 times, most recently from b6605fe to 2fd0c36 Compare April 19, 2023 10:23
@youknowone
Copy link
Member Author

This is rabbit hole. I touched builtin functions, method, module and every derived macros, and now reached bytecode CallMethod* doesn't working well with descriptors.

@youknowone
Copy link
Member Author

Split PRs to: #4886 #4898 #4899 #4900 #4901

@youknowone
Copy link
Member Author

@moreal This is not fully done, but could you give it a look? This is also related to PyMember design, so maybe you will be interested in.

@youknowone youknowone force-pushed the method branch 5 times, most recently from ae46438 to 2d5364d Compare April 29, 2023 13:57
@youknowone youknowone mentioned this pull request Apr 29, 2023
Merged
@youknowone
Copy link
Member Author

youknowone commented Apr 29, 2023 •
edited
Loading

Finally all cpython test suite tests pass

@youknowone youknowone marked this pull request as ready for review April 30, 2023 03:27
@youknowone
Copy link
Member Author

@seungha-kim @moreal Could you review this PR?

@youknowone youknowone requested a review from fanninpm April 30, 2023 03:27
@youknowone youknowone mentioned this pull request Apr 30, 2023
Closed
@youknowone youknowone changed the title method overhaul Method overhaul with static PyMethodDef Apr 30, 2023
moreal
moreal approved these changes Apr 30, 2023
View reviewed changes
Copy link
Contributor

@moreal moreal left a comment

Choose a reason for hiding this comment

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

I had tried to review it well but I'm not sure I've done a good job of reviewing long pieces of code. But I had read it once, and it builds and tests well, so I approve.

@youknowone youknowone merged commit 1dd5e84 into RustPython:main May 5, 2023
@youknowone youknowone deleted the method branch May 5, 2023 09:53
@youknowone youknowone mentioned this pull request May 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moreal moreal moreal approved these changes

@qingshi163 qingshi163 Awaiting requested review from qingshi163

@DimitrisJim DimitrisJim Awaiting requested review from DimitrisJim

@coolreader18 coolreader18 Awaiting requested review from coolreader18

@fanninpm fanninpm Awaiting requested review from fanninpm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@youknowone @moreal @blue-pandaa