| msg209122 - (view) |
Author: Tal Einat (taleinat) *  |
Date: 2014-01-24 21:35 |
| To quote where Larry Hastings added AC support for __new__ and __init__: " * __init__ and __new__ always take kwargs. * if the function signature doesn't accept keyword arguments, it calls _PyArg_NoKeywords(). " However, due to , many classes only do a _PyArg_NoKeywords if the type is the original class, but not for sub-classses. Examples are _random.Random (in which case I found a workaround) and many itertools classes, for which there are also tests that check this. One possibility is to simply allow these classes to also accept keyword arguments. It doesn't break backwards compatibility, and once there are proper argument names and doc-strings, why not? Otherwise, currently I have to make generated __new__ functions accept keyword arguments, and then wrap them with a function that checks _PyArg_NoKeywords only if the the type is the original class. |
|
|
| msg209124 - (view) |
Author: Larry Hastings (larry) * |
Date: 2014-01-24 21:44 |
| How about I add the "if type == " checks as per #1486663? |
|
|
| msg209127 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2014-01-24 22:05 |
| In general, that sounds like a reasonable suggestion to me. A quick search shows that most classes that call _PyArg_NoKeywords check the type first. I think we should strive for uniformity in this respect, so I'm +1 for your suggestion. I do see some classes which accept only positional arguments but where that check isn't in place. Examples: * itertools.tee just calls PyArg_ParseTuple without checking _PyArg_NoKeywords (is this a bug?) (this is fairly common, I think) * range always calls _PyArg_NoKeywords, without checking the type (is this a bug?) (this is rare) |
|
|
| msg209128 - (view) |
Author: Larry Hastings (larry) * |
Date: 2014-01-24 22:14 |
| I think it's a bug that they don't do the subclass check. In practice, nobody subclasses range and itertools.tee. But they should probably still be fixed. |
|
|
| msg209130 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2014-01-24 22:28 |
| Great. Most of the classes in itertools could use this. BTW, you'll need to supply a way to supply the C name of the type, to use in the type check. |
|
|
| msg209131 - (view) |
Author: Larry Hastings (larry) * |
Date: 2014-01-24 22:30 |
| Way ahead of you. The "class" directive will have to change: class name typedef type_object On the other hand, this means that Argument Clinic can automatically give the right type to the default self converter. So most people won't have to write custom self converter classes anymore. Argument Clinic giveth, and it taketh away. |
|
|
| msg209132 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2014-01-24 22:35 |
| +1 for that as well, I've written quite a few self converters for that reason. |
|
|
| msg209136 - (view) |
Author: Larry Hastings (larry) * |
Date: 2014-01-24 22:49 |
| How about this? I'm going to do a rollup patch today with three fixes: this, #20381, and changing the default filename for the "file" destination. |
|
|
| msg209168 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2014-01-25 08:55 |
| I've found two bugs: 1) In the type check, a '&' needs to be added before the type name. 2) Setting template_dict['self_type_object'] fails for module functions |
|
|
| msg209170 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2014-01-25 09:03 |
| Also, I believe the the type of the first argument passed to a method is a pointer to the typedef object, so a '*' needs to be added after the typedef name wherever it is used in such functions. |
|
|
| msg209176 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2014-01-25 09:36 |
| My last comment was wrong. There is a bug regarding the first argument to new methods; It should just remain a PyTypeObject*. |
|
|
| msg209177 - (view) |
Author: Larry Hastings (larry) * |
Date: 2014-01-25 09:38 |
| 1) That's not a bug, that's the API. If you used the dynamic API to create a type it wouldn't take the &. So I can't guess in advance what type it is, nor can I assume I always add the &. 2) Will fix. |
|
|
| msg209207 - (view) |
Author: Larry Hastings (larry) * |
Date: 2014-01-25 15:30 |
| The bug you cited is fixed in today's rollup patch, #20390. (I don't know how to denote the dependency between the two issues, maybe someone else can do that for me?) |
|
|
| msg209219 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2014-01-25 17:26 |
| I'm still seeing the first argument to a __new__ function as "groupbyobject *" instead of "PyTypeObject *". This causes the following error (for example): ./Modules/itertoolsmodule.c:112:34: error: no member named 'tp_alloc' in 'groupbyobject' gbo = (groupbyobject *)type->tp_alloc(type, 0); |
|
|
| msg209220 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2014-01-25 17:28 |
| To clarify my previous comment, I was referring to the first argument passed to the generated 'impl' function. Context: I'm attempting to convert 'itertools.groupby' in Modules/itertoolsmodule.c. |
|
|
| msg209221 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2014-01-25 17:33 |
| Also, I'm seeing this in the generated code for __new__ methods: if (({self_name} == {self_type_object}) && |
|
|
| msg209222 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2014-01-25 17:42 |
| To make reproducing these bugs easier, I'm attaching my partially converted version of Modules/itertoolsmodules.c, which has the buggy generated code inside. "Partially converted" means that I've only converted some of the functions requiring conversion. This file should be in a working state. |
|
|
| msg209258 - (view) |
Author: Larry Hastings (larry) * |
Date: 2014-01-26 00:18 |
| Can you try locally applying my patch for #20390 and seeing if you still have the problem? I tried applying it to the itertoolmodule.c you attached to the issue and it seemed to fix the problem. |
|
|
| msg209322 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2014-01-26 14:21 |
| Using the latest clinic.py from default branch, everything in itertools works like a charm :) |
|
|