msg207948 - (view) |
Author: Georg Brandl (georg.brandl) *  |
Date: 2014-01-12 11:05 |
This is a request to be able to name C arguments differently from Python arguments.
Two reasons:
* like for function renaming, some Python argument names are reserved in C ("default" for example, used in sys.getsizeof())
* sometimes the function uses 'O' in PyArg_ParseTuple and then converts the object itself with a nontrivial process. Usually the converted variable has the name of the Python argument, while the temporary object is named "obj_foo" or "foo_obj". When converting to Argument Clinic, we have to name the object "foo" and find a new name for the converted "foo", which leads to code churn and less beautiful code.
|
msg207949 - (view) |
Author: Georg Brandl (georg.brandl) *  |
Date: 2014-01-12 11:07 |
OK, I see that AC automatically handles case 1, great!
Still you have to decide if you like the code churn caused by case 2.
|
msg207950 - (view) |
Author: Georg Brandl (georg.brandl) *  |
Date: 2014-01-12 11:14 |
One more: I can't have an argument called "args".
|
msg208003 - (view) |
Author: Zachary Ware (zach.ware) *  |
Date: 2014-01-13 03:51 |
I second this request. Every PyArg_ParseTupleAndKeywords-using function in _winapi has a keyword argument "overlapped" which is stuffed into a C variable "use_overlapped", while the local variable "overlapped" is used for an Overlapped object, which makes for a big hairy diff. There are also several PyArg_ParseTuple-using functions that have the same kind of name mismatch, but since the names don't really mean anything on the Python side when they're positional-only, I don't feel too bad about changing them to make them make sense (or to avoid churn).
Although, for this:
Georg Brandl wrote:
> sometimes the function uses 'O' in PyArg_ParseTuple and then converts the object itself with a nontrivial process.
This kind of case is a good candidate for considering a custom converter; posixmodule.c has a good example with a path_t converter that I used for a guide in writing one for HKEY in winreg.c. Of course, if that 'nontrivial process' is only done once in one function, a custom converter doesn't make a whole lot of sense, but a rename does.
|
msg208005 - (view) |
Author: Georg Brandl (georg.brandl) *  |
Date: 2014-01-13 06:39 |
> There are also several PyArg_ParseTuple-using functions that have the
> same kind of name mismatch, but since the names don't really mean
> anything on the Python side when they're positional-only, I don't feel
> too bad about changing them to make them make sense (or to avoid churn).
Although now is a good time to ensure sensible argument names (I usually look at the docs to find the documented ones), so that switching the function to keyword arg support is basically just a removal of '/'.
|
msg208065 - (view) |
Author: Ryan Smith-Roberts (rmsr) * |
Date: 2014-01-14 02:22 |
Georg Brandl wrote:
> Although now is a good time to ensure sensible argument names (I
> usually look at the docs to find the documented ones), so that
> switching the function to keyword arg support is basically just a
> removal of '/'
I started doing this almost without thinking while converting the socket module. I think matching argument names against the online docs is an important part of AC conversion that has gone unaddressed. I am working on an email to python-dev about making it explicit policy.
A way to avoid code churn that just occurred to me is to retain existing variable declarations (for users of PyArgs_ParseTuple) and initialize them using the new function arguments. For the self argument and METH_O users one can introduce a new declaration similarly.
|
msg208230 - (view) |
Author: Larry Hastings (larry) *  |
Date: 2014-01-16 03:16 |
I don't see the big win from this. You can rename variables in C any way you like. "functionname as c_basename" is to fix otherwise unavoidable collisions; this seems like a nice-to-have. And I already have a lot on my plate. I could consider it later but the priority for this is below converting functions.
Georg: I remind you that nearly every parsing function already has a variable called "args". To allow you to have a parameter called "args" would mean I'd have to have really complicated internal structure, where
the variable I declare in the parsing function would have a different
name from the parameter to the impl function. Do you really have a
burning, nearly incandescent need for this feature?
Zachary: You could minimize the size of the diff by using nested scopes:
_impl(...)
/*[clinic end generated code: checksum=...]*/
{
int use_overlapped = overlapped;
{
LP_OVERLAPPED overlapped;
...
|
msg208240 - (view) |
Author: Georg Brandl (georg.brandl) *  |
Date: 2014-01-16 05:51 |
I guess custom converters are the answer here.
BTW, the "args" argument already works fine since adding it to c_keywords. No need for a complicated structure :)
|
msg208262 - (view) |
Author: Ryan Smith-Roberts (rmsr) * |
Date: 2014-01-16 09:14 |
The use case is primarily to minimize code churn for the derby, but since you're the one (heroically) doing the code review it's really your call. I whipped up a quick patch for this feature, and even if you remove c_name from __init__ I think it's still a small cleanup.
|
msg208286 - (view) |
Author: Zachary Ware (zach.ware) *  |
Date: 2014-01-16 16:19 |
The nested scope trick makes me happy. There are still a lot of cases in the conversions I've worked on where a variable had to be renamed due to having documented a different name for the parameter, but in the vast majority of those cases the new name makes more sense anyway.
|
msg208393 - (view) |
Author: Larry Hastings (larry) *  |
Date: 2014-01-18 09:54 |
I have a couple big patches incoming, over the next couple of days. Here's a sneak-peek of my todo list:
* buffer support just went in (#20287)
* all builtins support landing maybe tomorrow (#20260)
* suppress the "self" parameter on impl for METH_STATIC
* allow variables to be renamed between parsing and impl functions ***
* allow passing in args and kwargs into impl (#20291)
The one I triple-starred is going to change the code you hacked up a *lot*. I'm not going to look at this patch before then, because that change will break the patch completely.
But I'll make you a deal. If you try again *after* all of the above lands, I'll look at it, and if it's fine it can probably go in.
I don't know if I said, but the obvious syntax is "foo as bar". "as" should bind more tightly than the colon for the converter.
|
msg208417 - (view) |
Author: Ryan Smith-Roberts (rmsr) * |
Date: 2014-01-18 21:12 |
I believe the feature you starred resolves this enhancement issue, in which case my patches are obsolete. And yes, the 'as' syntax makes a lot more sense. Here's how I hope it works (the arg parsing wrapper remains unchanged):
foo: int
...
_impl(..., int foo)
can become
foo: int as bar
...
_impl(..., int bar)
|
msg208419 - (view) |
Author: Larry Hastings (larry) *  |
Date: 2014-01-18 22:14 |
On 01/18/2014 01:12 PM, Ryan Smith-Roberts wrote:
> Ryan Smith-Roberts added the comment:
>
> I believe the feature you starred resolves this enhancement issue, in which case my patches are obsolete. And yes, the 'as' syntax makes a lot more sense. Here's how I hope it works (the arg parsing wrapper remains unchanged):
>
> foo: int
> ...
> _impl(..., int foo)
>
> can become
>
> foo: int as bar
> ...
> _impl(..., int bar)
Nope, not what I wanted. I said "as" binds more tightly than ":":
foo as bar: int
But if you're dropping the patch I guess it's irrelevant.
//arry/
|
msg208421 - (view) |
Author: Ryan Smith-Roberts (rmsr) * |
Date: 2014-01-18 22:44 |
Yes, that makes more sense. The specific syntax is nearly irrelevant compared to the feature existing at all. You still haven't indicated whether your starred feature matches the *result* that I outlined. Does it?
|
msg208702 - (view) |
Author: Tal Einat (taleinat) *  |
Date: 2014-01-21 20:41 |
+1 for this. It would save a bit of manual conversion work, and would significantly reduce the size and complexity of the resulting patches.
|
msg212390 - (view) |
Author: Zachary Ware (zach.ware) *  |
Date: 2014-02-27 22:06 |
This was added by #20456 (a couple weeks ago).
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:56 | admin | set | github: 64426 |
2014-02-27 22:06:57 | zach.ware | set | status: open -> closed
superseder: Argument Clinic rollup patch, 2014/01/31 components:
+ Demos and Tools versions:
+ Python 3.4 messages:
+ msg212390 type: enhancement resolution: fixed stage: resolved |
2014-01-21 20:41:38 | taleinat | set | messages:
+ msg208702 |
2014-01-21 20:30:57 | zach.ware | link | issue20327 superseder |
2014-01-21 20:30:45 | zach.ware | set | nosy:
+ taleinat
|
2014-01-18 23:21:05 | serhiy.storchaka | set | nosy:
- serhiy.storchaka
|
2014-01-18 22:44:54 | rmsr | set | messages:
+ msg208421 |
2014-01-18 22:14:33 | larry | set | messages:
+ msg208419 |
2014-01-18 21:12:12 | rmsr | set | messages:
+ msg208417 |
2014-01-18 09:54:45 | larry | set | messages:
+ msg208393 |
2014-01-16 16:19:34 | zach.ware | set | messages:
+ msg208286 |
2014-01-16 09:16:36 | rmsr | set | files:
+ argument_clinic_ensure_legal_cleanup.patch |
2014-01-16 09:14:56 | rmsr | set | files:
+ argument_clinic_rename_c_variable.patch keywords:
+ patch messages:
+ msg208262
|
2014-01-16 05:51:23 | georg.brandl | set | messages:
+ msg208240 |
2014-01-16 03:16:19 | larry | set | messages:
+ msg208230 |
2014-01-14 02:22:42 | rmsr | set | messages:
+ msg208065 |
2014-01-14 00:19:03 | rmsr | set | nosy:
+ rmsr
|
2014-01-13 06:39:30 | georg.brandl | set | messages:
+ msg208005 |
2014-01-13 03:51:12 | zach.ware | set | nosy:
+ zach.ware messages:
+ msg208003
|
2014-01-12 11:14:29 | georg.brandl | set | messages:
+ msg207950 |
2014-01-12 11:07:38 | georg.brandl | set | messages:
+ msg207949 |
2014-01-12 11:06:47 | georg.brandl | set | nosy:
+ serhiy.storchaka
|
2014-01-12 11:05:36 | georg.brandl | create | |