classification
Title: Argument Clinic: rename arguments in generated C?
Type: enhancement Stage: resolved
Components: Demos and Tools Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder: Argument Clinic rollup patch, 2014/01/31
View: 20456
Assigned To: larry Nosy List: georg.brandl, larry, rmsr, taleinat, zach.ware
Priority: normal Keywords: patch

Created on 2014-01-12 11:05 by georg.brandl, last changed 2014-02-27 22:06 by zach.ware. This issue is now closed.

Files
File name Uploaded Description Edit
argument_clinic_rename_c_variable.patch rmsr, 2014-01-16 09:14 with c_name on __init__ review
argument_clinic_ensure_legal_cleanup.patch rmsr, 2014-01-16 09:16 no support for actually renaming the c variable review
Messages (16)
msg207948 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2014-01-12 11:14
One more: I can't have an argument called "args".
msg208003 - (view) Author: Zachary Ware (zach.ware) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2014-02-27 22:06
This was added by #20456 (a couple weeks ago).
History
Date User Action Args
2014-02-27 22:06:57zach.waresetstatus: 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:38taleinatsetmessages: + msg208702
2014-01-21 20:30:57zach.warelinkissue20327 superseder
2014-01-21 20:30:45zach.waresetnosy: + taleinat
2014-01-18 23:21:05serhiy.storchakasetnosy: - serhiy.storchaka
2014-01-18 22:44:54rmsrsetmessages: + msg208421
2014-01-18 22:14:33larrysetmessages: + msg208419
2014-01-18 21:12:12rmsrsetmessages: + msg208417
2014-01-18 09:54:45larrysetmessages: + msg208393
2014-01-16 16:19:34zach.waresetmessages: + msg208286
2014-01-16 09:16:36rmsrsetfiles: + argument_clinic_ensure_legal_cleanup.patch
2014-01-16 09:14:56rmsrsetfiles: + argument_clinic_rename_c_variable.patch
keywords: + patch
messages: + msg208262
2014-01-16 05:51:23georg.brandlsetmessages: + msg208240
2014-01-16 03:16:19larrysetmessages: + msg208230
2014-01-14 02:22:42rmsrsetmessages: + msg208065
2014-01-14 00:19:03rmsrsetnosy: + rmsr
2014-01-13 06:39:30georg.brandlsetmessages: + msg208005
2014-01-13 03:51:12zach.waresetnosy: + zach.ware
messages: + msg208003
2014-01-12 11:14:29georg.brandlsetmessages: + msg207950
2014-01-12 11:07:38georg.brandlsetmessages: + msg207949
2014-01-12 11:06:47georg.brandlsetnosy: + serhiy.storchaka
2014-01-12 11:05:36georg.brandlcreate