bpo-37499: add test functions for each calling convention #14795
Conversation
|
CC @pganssle |
There was a problem hiding this comment.
Sorry for the long delay in reviewing this.
I am ever-so-slightly worried that we're moving these all over to being method calls instead of function calls, but I can't think of any good reason why it would undermine the point of the tests.
I personally like the namespacing of a method, so I say leave it as is.
Also, it's not causing any new failures, but test_gdb is now spitting out a bunch of errors or warnings or something that seem very related:
$ ./python -m test test_gdb
Run tests sequentially
0:00:00 load avg: 1.84 [1/1] test_gdb
Function "CallTest_varargs" not defined.
Function "CallTest_varargs" not defined.
Function "CallTest_varargs_keywords" not defined.
Function "CallTest_varargs_keywords" not defined.
Function "CallTest_fastcall" not defined.
Function "CallTest_fastcall" not defined.
Function "CallTest_fastcall_keywords" not defined.
Function "CallTest_fastcall_keywords" not defined.
Function "CallTest_noargs" not defined.
Function "CallTest_noargs" not defined.
Function "CallTest_onearg" not defined.
Function "CallTest_onearg" not defined.
Function "CallTest_call" not defined.
Function "CallTest_call" not defined.
Function "CallTest_call" not defined.
Function "CallTest_call" not defined.
test_gdb passed in 37 sec 821 ms
== Tests result: SUCCESS ==
1 test OK.
Total duration: 37 sec 828 ms
Tests result: SUCCESS
Modules/_testcapimodule.c
Outdated
| } | ||
|
|
||
| static PyObject * | ||
| CallTest_fastcall_keywords(PyObject *self, PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames) |
There was a problem hiding this comment.
PEP 7: Line too long,
| CallTest_fastcall_keywords(PyObject *self, PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames) | |
| CallTest_fastcall_keywords(PyObject *self, PyObject *const *args, | |
| Py_ssize_t nargs, PyObject *kwnames) |
Lib/test/test_call.py
Outdated
| @@ -321,6 +321,9 @@ def static_method(): | |||
|
|
|||
| @cpython_only | |||
| class FastCallTests(unittest.TestCase): | |||
| from _testcapi import CallTest as C | |||
| c = C() | |||
There was a problem hiding this comment.
Hm, this is actually quite confusing, c and C look very similar, I did not even realize that you were alternating between instance methods and class methods.
Can we use more distinct-looking names for these? How about:
from _testcapi import CallTest as CT
ci = CT() Something like this?
There was a problem hiding this comment.
I'd prefer CallTest and calltest.
There was a problem hiding this comment.
I still find it very hard to visually distinguish between a sea of references like this:
calltest.blah
CallTest.blah
calltest.blahCan it be something like ct_inst and CallTest?
ct_inst.blah
CallTest.blah
ct_inst.blahEven call_test and CallTest would be fine:
call_test.blah
CallTest.blah
call_test.blah
|
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 hope to look at this at this week's sprint. To make it faster, I'll push changes directly to this branch. |
This has to do with the fact that we're importing these functions from an external module ( |
|
I did something similar – testing that C callables can be called fro Python code. See GH-15776 |
|
Regarding the formatting in |
|
That's one point where I disagree. The aligned code looks neater, but IMO it doesn't help readability much, and makes larger diffs wen it needs to be updated. |
Adapted by Petr Viktorin from python#14795 All bugs are Petr's :)
https://bugs.python.org/issue37499