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.
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
Derby #17: Convert 49 sites to Argument Clinic across 13 files #64384
Comments
This issue is part of the Great Argument Clinic Conversion Derby, This issue asks you to change the following bundle of files: Talk to me (larry) if you only want to attack part of a bundle. For instructions on how to convert a function to work with Argument |
Antoine Pitrou took care of cryptmodule.c. |
Here is the patch for Modules/resource.c. However, I can not convert this method signature: if (!PyArg_ParseTuple(args, _Py_PARSE_PID "i|(OO):prlimit",
&pid, &resource, &curobj, &maxobj)) _Py_PARSE_PID can be 'i' or 'l' or 'L'. |
Here is the patch for Modules/gcmodule.c. |
Here is the patch for Python/marshal.c. A couple of issues:
So I changed it to byte.
version: int(c_default="Py_MARSHAL_VERSION") = marshal.version -> not possible (unless I put "import marshal" in Tools/clinic/clinic.py). So I gave it a raw value, 2. version: int(c_default="Py_MARSHAL_VERSION") = 2 |
Someone else came up with this hack using a custom converter: class PID_converter(int_converter):
format_unit = '" _Py_PARSE_PID "' Also, for marshal.version, soon you will be able to. This syntax won't work now but it will soon: version: int(c_default="Py_MARSHAL_VERSION") = version |
Keep track of bpo-20226, that's the issue where I'm adding improved support for simple expressions like "version". |
With little modification: class PID_converter(int_converter):
type = 'pid_t'
format_unit = '" _Py_PARSE_PID "' It works. Thanks! But I got unnecessary empty string: if (!PyArg_ParseTuple(args, "" _Py_PARSE_PID "i:prlimit", &pid, &resource)) It should be: if (!PyArg_ParseTuple(args, _Py_PARSE_PID "i:prlimit", &pid, &resource)) Anyway, that is trivial problem. Now I hit a hard one. How do you convert this signature?
I create custom converters: +class TupleOpen_converter(object_converter): and +/[clinic input] But I got invalid argument size in the generated code. + case 2: Any idea, Larry? |
This is known bug in prlimit (bpo-20191). |
Here is the patch for typeobject. I didn't convert the new method. It's so complicated. It counts whether how many arguments or keywords you pass. It asserts the args before parsing the args. I don't think clinic supports this. This is the code: /* Special case: type(x) should return x->ob_type */
{
const Py_ssize_t nargs = PyTuple_GET_SIZE(args);
const Py_ssize_t nkwds = kwds == NULL ? 0 : PyDict_Size(kwds);
if (PyType_CheckExact(metatype) && nargs == 1 && nkwds == 0) {
PyObject *x = PyTuple_GET_ITEM(args, 0);
Py_INCREF(Py_TYPE(x));
return (PyObject *) Py_TYPE(x);
}
/* SF bug 475327 -- if that didn't trigger, we need 3
arguments. but PyArg_ParseTupleAndKeywords below may give
a msg saying type() needs exactly 3. */
if (nargs + nkwds != 3) {
PyErr_SetString(PyExc_TypeError,
"type() takes 1 or 3 arguments");
return NULL;
}
}
|
Here is the patch for longobject. There are two sites which I couldn't convert. The first is the constructor which is complicated. The other one is __round__ which clinic explicitly does not support. |
Here is the patch for listobject. A couple of thoughts:
if (!PyArg_ParseTuple(args, "O|O&O&:index", &v,
_PyEval_SliceIndex, &start,
_PyEval_SliceIndex, &stop))
return NULL; It's bloody difficult. If I make start (and stop) as PyObject, I get a lot of pointer warning conversion. The closest I can get is making start (and stop) as Py_ssize_t but it changes the behaviour. a.index(0, -4*sys.maxsize, 4*sys.maxsize) will throw OverflowError.
This is the clinic input: list.__init__ self: PyListObject Without None, it will be an error in clinic process.
static int
list___init__(PyObject *self, PyObject *args, PyObject *kwargs)
{
int return_value = -1;
int group_right_1 = 0;
PyObject *sequence = NULL;
if (!_PyArg_NoKeywords("__init__", kwargs))
goto exit;
switch (PyTuple_GET_SIZE(args)) {
case 0:
break;
case 1:
if (!PyArg_ParseTuple(args, "O:__init__", &sequence))
return NULL;
group_right_1 = 1;
break;
.... |
/*[python input] class slice_index_converter(CConverter):
type = 'Py_ssize_t'
converter = '_PyEval_SliceIndex' [python start generated code]*/
|
I'll fix the "return NULL" problem. However, you are using optional groups in the list __init__. The original doesn't use them. Please stop using optional groups in functions that don't require them. |
Thanks, Serhiy, for the pointer. Now, I am able to convert the method using _PyEval_SliceIndex function. Sorry, Larry. I used optional groups in __init__ because I didn't know about unspecified. Here is the updated patch for listobject. One thought, for list.index method, I use this: stop: slice_index(c_default="Py_SIZE(self)") = unspecified Then the signature will be: "index(value, [start=0, [stop=unspecified]])\n" Somehow I prefer more explanatory signature: "index(value, [start=0, [stop=size of list]])\n" Anyway, this is a trivial thing. |
Here is the patch for floatobject. I did not convert 2 sites. The first is the round method which clinic explicitly does not support. The second one is the new method. This is the snippet of new method: float_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
{
PyObject *x = Py_False; /* Integer zero */
static char *kwlist[] = {"x", 0};
.... If I clinic this method, I could not put custom code before parsing arguments code anymore. This could affect the performance or may not be correct at all. |
This is updated patch for marshal incorporating the fixes from clinic. Now, I can use 'bytes' named argument and default value of marshal.version instead of hardcoded number. |
Updated marshal patch. I just learned we don't need to release pybuffer (y*) manually. |
Here is the updated patch for resource module based on Zachary's comment. |
Here is the updated patch for type module based on Zachary's comment. However, I can not convert this method. {"__subclasshook__", object_subclasshook, METH_CLASS | METH_VARARGS, static PyObject *
object_subclasshook(PyObject *cls, PyObject *args)
{
Py_RETURN_NOTIMPLEMENTED;
} >>> type.__subclasshook__()
NotImplemented
>>> type.__subclasshook__('cutecat')
NotImplemented
>>> type.__subclasshook__('cutecat', 1, [])
NotImplemented |
Here is the updated patch for list module based on Zachary and Serhiy's reviews. |
Forgot to say that, in list module, anything is convertable except __getitem__. |
Here is the updated patch for marshal module based on Zachary's review. |
Here is the updated patch for float object based on Zachary and Serhiy's reviews. Some methods that can not be converted are: __getnewargs__, __round__, float_new. So these files are ready for Python 3.4: resource, typeobject, listobject, and floatobject. These files are not ready yet: gc, longobject. |
Attaching a conglomerate patch consisting of clinic_resource_v2.patch, clinic_typeobject_v2.patch, clinic_listobject_v3.patch, clinic_marshal_v4.patch, and clinic_floatobject_v2.patch, with updated (post-bpo-20326) clinic output, for easier review. |
Here is the updated patch addressing Zachary's reviews (Thanks!). However, there are some reviews that I could not implement.
I can not synchronize struct rlimit and NULL return values.
I tried it but it was like opening pandora box. It's too much effort to surpress compile errors and warnings. And casting PyFloatObject to PyObject in many places, such as functions, macros, makes me nervous. I think this one deserves a dedicated ticket. |
Vajrasky Kok wrote:
Looking again, that one is non-trivial, but still doable. You just need a "this means an error happened" value to initialize rl to, and return that value instead of NULL.
It didn't look too bad to me. There are already several places where a value is cast back and forth between PyObject and PyFloatObject. Giving 'self' the right type allows a couple of casts to be removed, and the ones that have to be added are almost exclusively in calls to PyFloat_AsDouble or the CONVERT_TO_DOUBLE macro (which looks like it can just do the casts itself without much issue). I would vote for making PyFloat_AsDouble expect PyFloatObject instead of PyObject, but since (I think?) it looks like it's part of the stable ABI, I'm not sure if that would fly. See http://hg.python.org/sandbox/zware/rev/51473d8c23f8 for a patch on your patch that uses PyFloatObject, compiles cleanly (on win32, at least), and passes relevant tests (though I haven't run the full test suite on this yet; it takes forever on this PC). I have a few more review comments that I hope to get posted later this evening. The patch is looking pretty good overall, though! |
Look like this has never been applied, any reason for this? Tip has probably highly diverged, but I may try to rebase it, should I? |
New changeset c62352ec21bc by Victor Stinner in branch 'default': |
Julien, to help push these changes forward, I suggest start by rebasing and reviewing the conglomerate patch, since it seems that was almost ready. Reading through the thread, it seems the current status is:
|
Zach’s comments on float seem to have been addressed (https://bugs.python.org/review/20185/diff2/10940:10949/Objects/floatobject.c). The comments on the resource module were about return converters: <http://bugs.python.org/review/20185/diff2/10817:10949/Modules/resource.c\>. One has been addressed. The other was about the rlimit2py() function, used in two places. One of these is resource.prlimit(), which is not converted to Arg Clinic in the patch. If we don’t convert it, I agree that leaving rlimit2py() as it is is simplest. Given that the current prlimit() arg parsing is buggy (bpo-20191), it would be good to convert it to Arg Clinic in the process of solving the bug, but that could be done as a separate step. Tangentially, the rlimit2py() function has special HAVE_LONG_LONG handling. This is no longer needed in 3.6; see bpo-27961. But again, I think it is better to review and commit the existing patch than to keep perfecting it :) |
Victor, you perhaps could just use NULL as default: + source: object = NULL |
I finished reviewing the most recent patches and left some comments. Perhaps it is worth splitting the conglomerate patch up. I don’t see any point holding back some modules while things are tweaked in unrelated files. My biggest concern is casting function pointers, especially when the number of parameters is different than expected. It would be good to fix or discuss that more before going ahead with floatobject.c. |
Martin, you are the one who looked at these patches for last three years. Do you want to take this issue to you and update Vajrasky's patches? Now there is good performance argument for converting builtins to Argument Clinic. |
Will keep this in mind, but my time is rather limited, so I may not get to it (and I wouldn’t want to discourage other people from working on it) |
Updated patch for longobject.c addresses Martin's comments. |
longobject_v5 looks good to me |
New changeset aa7ac93d23b2 by Serhiy Storchaka in branch 'default': |
Updated patch for the gc module is synchronized with current sources and addresses Martin's comments. The only problem with this patch is that it exposes the default value for gc.collect() argument as constant 2 in Python instead of actual NUM_GENERATIONS-1. The latter value can't be exposed in Python. On other side it is 2 in the documentation, and it is not so easy to change the value of NUM_GENERATIONS. |
New changeset 50958e13c833 by Serhiy Storchaka in branch 'default': |
Here is a set of patches based on issue20185_conglomerate_v4.diff. They are synchronized with current sources. Addressed Martin's comments,converted list.index(), float.__round__() and resource.prlimit() and made other minor changes. |
I rebased the conglomerate patch onto current master, and there's only three methods left, in floatobject.c, it's three methods inside a #if 0, it does not looks interesting to merge it (30 insertions, 12 deletions), so I'm closing this issue. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: