Skip to content

Method overhaul with static PyMethodDef - #4873

Merged
youknowone merged 6 commits into
RustPython:mainfrom
youknowone:method
May 5, 2023
Merged

youknowone merged 6 commits into
RustPython:mainfrom
youknowone:method

Conversation

@youknowone

@youknowone youknowone commented Apr 12, 2023 •

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member Author

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

@youknowone

Copy link
Copy Markdown
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 commented Apr 29, 2023 •

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Member Author

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

@youknowone
youknowone requested a review from fanninpm April 30, 2023 03:27
@youknowone youknowone changed the title method overhaul Method overhaul with static PyMethodDef Apr 30, 2023

@moreal moreal 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.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants