Skip to content
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

gh-104469 Convert_testcapi/vectorcall.c to use AC #106557

Merged
merged 6 commits into from
Jul 9, 2023
Merged

gh-104469 Convert_testcapi/vectorcall.c to use AC #106557

merged 6 commits into from
Jul 9, 2023

Conversation

littlebutt
Copy link
Contributor

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@bedevere-bot bedevere-bot mentioned this pull request Jul 9, 2023
Closed
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Jul 9, 2023 •
edited
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@corona10 corona10 self-requested a review July 9, 2023 03:13
sobolevn
sobolevn reviewed Jul 9, 2023
View reviewed changes
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

I don't think that we need to convert test_ prefixed functions. They are executed as tests directly.

@corona10
Copy link
Member

corona10 commented Jul 9, 2023

I don't think that we need to convert test_ prefixed functions. They are executed as tests directly.

Well, I am okay with it. We already did.
#104720

corona10
corona10 requested changes Jul 9, 2023
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

First thing, method names are wrong.
Please change method names and then executing the make clinic

Modules/_testcapi/vectorcall.c Outdated
@@ -25,18 +29,23 @@ fastcall_args(PyObject *args, PyObject ***stack, Py_ssize_t *nargs)
return 0;
}

/*[clinic input]
_testcapi.test_pyobject_fastcalldict
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_testcapi.test_pyobject_fastcalldict
_testcapi.pyobject_fastcalldict

Modules/_testcapi/vectorcall.c Outdated
@@ -52,17 +61,23 @@ test_pyobject_fastcalldict(PyObject *self, PyObject *args)
return PyObject_VectorcallDict(func, stack, nargs, kwargs);
}

/*[clinic input]
_testcapi.test_pyobject_vectorcall
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_testcapi.test_pyobject_vectorcall
_testcapi.pyvectorcall_call

Modules/_testcapi/vectorcall.c Outdated
_testcapi.test_pyobject_vectorcall
func: object
func_args: object
kwnames: object = NULL
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
kwnames: object = NULL
kwnames: object = None

Modules/_testcapi/vectorcall.c Outdated
@@ -103,17 +118,19 @@ function_setvectorcall(PyObject *self, PyObject *func)
Py_RETURN_NONE;
}

/*[clinic input]
_testcapi.test_pyvectorcall_call
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_testcapi.test_pyvectorcall_call
_testcapi.pyvectorcall_call

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@littlebutt
Copy link
Contributor Author

@corona10 Thank you for your suggestions and I fixed these method names. But the complaints still exist because of missing arguments in test cases.

@corona10
Copy link
Member

corona10 commented Jul 9, 2023

@corona10 Thank you for your suggestions and I fixed these method names. But the complaints still exist because of missing arguments in test cases.

According to CI, it looks not, did you rebuild the code?

@littlebutt
Copy link
Contributor Author

@corona10 Thank you for your suggestions and I fixed these method names. But the complaints still exist because of missing arguments in test cases.

According to CI, it looks not, did you rebuild the code?

No, I didn't. I just ran rt.bat -q test_capi directly after generating .c.h file😂. Thank you for help and I learnt a lot.

corona10
corona10 approved these changes Jul 9, 2023
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm

@corona10 corona10 merged commit d137c2c into python:main Jul 9, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sobolevn sobolevn sobolevn left review comments

@corona10 corona10 corona10 approved these changes

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@littlebutt @bedevere-bot @corona10 @sobolevn