homepage

This issue is part of the Great Argument Clinic Conversion Derby,
where we're trying to convert as much of Python 3.4 to use
Argument Clinic as we can before Release Candidate 1 on January 19.
This issue asks you to change the following bundle of files:
 Objects/tupleobject.c: 2 sites
 Objects/memoryobject.c: 2 sites
 Objects/descrobject.c: 2 sites
 Objects/complexobject.c: 2 sites
 Modules/_operator.c: 2 sites
 Modules/_opcode.c: 2 sites
 Modules/_lsprof.c: 2 sites
 Modules/_heapqmodule.c: 2 sites
 Objects/weakrefobject.c: 1 sites
 Objects/structseq.c: 1 sites
 Objects/rangeobject.c: 1 sites
 Objects/object.c: 1 sites
 Objects/moduleobject.c: 1 sites
 Objects/funcobject.c: 1 sites
 Objects/fileobject.c: 1 sites
 Objects/enumobject.c: 1 sites
 Objects/codeobject.c: 1 sites
 Objects/boolobject.c: 1 sites
 Modules/symtablemodule.c: 1 sites
 Modules/mathmodule.c: 1 sites
 Modules/_tracemalloc.c: 1 sites
 Modules/_io/_iomodule.c: 1 sites
 Modules/_csv.c: 1 sites
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
Clinic, read the "howto":
 http://docs.python.org/dev/howto/clinic.html 

Looking at _csv.c, I see a few functions using PyArg_UnpackTuple. They should be converted too, no?

Attached part 1 of mathmodule (17 functions).
I'm looking forward to a suggestion for handling the rest (see FUNC1/1A/2 macros :)

1)
Wow. I never knew about PyArg_UnpackTuple. You're right, those should be converted too. Hooray, more entry points to convert.
I'll write something up for the howto about UnpackTuple.
I just did a quick check, and there are 96 entry points (by my count) that use PyArg_UnpackTuple(). Shall I create Derby issues #19 and #20, or do you have a better idea?
2) For FUNC1 / 1A / 2 macros: right now you'd have to just copy and paste over and over. There might be something you could do with a [python] block where you automatedly reuse the existing sigantures. I was thinking about having Clinic support it directly, maybe with the syntax:
/*[clinic input]
func_name = existing_func_name
docstring goes here
[...]*/
You'd skip the parameters and the return annotation. You could only reuse functions from the current file. Would that be a big boon to you?

> Wow. I never knew about PyArg_UnpackTuple. You're right, those 
> should be converted too. Hooray, more entry points to convert.
> I'll write something up for the howto about UnpackTuple.
One thing to note is that (at least in math) many instances of UnpackTuple could have been replaced by ParseTuple. See for example math_hypot: it uses UnpackTuple to get two objects, and then immediately calls PyFloat_AsDouble on them. I've converted these using 'd' and not 'O' specifiers.
> I just did a quick check, and there are 96 entry points (by my count) 
> that use PyArg_UnpackTuple(). Shall I create Derby issues #19 and 
> #20, or do you have a better idea?
Probably better to add them to the issues that cover their modules, otherwise people might get confused.
> 2) For FUNC1 / 1A / 2 macros: right now you'd have to just copy and 
> paste over and over. There might be something you could do with a 
> [python] block where you automatedly reuse the existing sigantures. 
> I was thinking about having Clinic support it directly, maybe with 
> the syntax:
> /*[clinic input]
> func_name = existing_func_name
> 
> docstring goes here
> [...]*/
> 
> You'd skip the parameters and the return annotation. You could only 
> reuse functions from the current file. Would that be a big boon to you?
That sounds good.
On the other hand, if clinic expanded cpp macros we could... *:-)

OK, here's a patch for _csv. Two problems here:
First problem is the __new__ method of the Dialect class:
* it has no docstring and no methoddef entry
* it is a class method, but the first arg is conventionally called "type"
I tried to hack something into clinic with a new decorator, but it may not be how you want it to look, take care.
Second problem is the functions reader(), writer(), register_dialect(): they parse their *args but pass their **kwargs through to another class.
Is there anything like a "**kwds" argument specifier?
BTW, for a module like _csv that is exported through a Python module named csv, should we use the "real" or the "nice" module name?

Tried to tackle symtable -- it uses an O& converter. The clinic howto says
``'O&'`` ``object(converter='name_of_c_function')``
but
Traceback (most recent call last):
 File "Tools/clinic/clinic.py", line 2817, in <module>
 sys.exit(main(sys.argv[1:]))
 File "Tools/clinic/clinic.py", line 2813, in main
 parse_file(filename, output=ns.output, verify=not ns.force)
 File "Tools/clinic/clinic.py", line 1116, in parse_file
 cooked = clinic.parse(raw)
 File "Tools/clinic/clinic.py", line 1066, in parse
 parser.parse(block)
 File "Tools/clinic/clinic.py", line 2109, in parse
 self.state(line)
 File "Tools/clinic/clinic.py", line 2378, in state_parameter
 converter = dict[name](parameter_name, self.function, value, **kwargs)
 File "Tools/clinic/clinic.py", line 1403, in __init__
 self.converter_init(**kwargs)
TypeError: converter_init() got an unexpected keyword argument 'converter'

_tracemalloc converted.
Its existing docstrings did use the
func(arg: argtype) -> rettype
convention. Is there a way in clinic to retain that?

Here's _iomodule. _io.open has a whopping 100-line docstring, which is ... unfortunate ... to have duplicated in the file :)

And _heapq. No problems there, except that it also used "->" return annotations in the docstring.

And lsprof.

OK, new patches coming in.

Actually I put all I have in one. Rietveld doesn't care.
The mathmodule still awaits some kind of solution for the macro atrocities.
Objects will be attacked next.

New changeset 060cfd049d14 by Stefan Krah in branch 'default':
Issue #20186: memoryobject.c: add function signatures.
http://hg.python.org/cpython/rev/060cfd049d14 

memoryobject.c is converted with a minimal patch (I would like to keep
100% code coverage for the file).

All the Derby patches should only go into trunk at this point.

Attached is an updated patch for Modules/mathmodule.c.
This is based on Georg's patch, updated to apply to current 3.5, with several improvements:
* replaced legacy converters
* converted math.ceil() and math.floor() functions
* converted the new math.gcd() and math.isclose() functions
* AC generated code in separate file: Modules/clinic/mathmodule.c.h
* this patch doesn't change any internal variable names in the C code

Should Argument Clinic conversion patches still be against the 'default' branch, and not 3.5, even though they don't include any functionality changes?

Attached is an AC conversion patch for Objects/enumobject.c.
Note that this file contains the implementations of the 'enumerate' and 'reversed' classes, but *not* the 'Enum' class.
This is based on the 3.5 branch.

Attached is a patch for all of _operator except for itemgetter, attrgetter and methodcaller. The entire test suite passes after applying this patch.
Using AC has allowed the removal of all of the cryptic "spam*()" macros in the code, making it much more straightforward. In terms of readability, IMO this is a great improvement.
I skipped itemgetter, attrgetter and methodcaller since they all support passing a variable number of arguments and treating those as a sequence of values.

Is there any performance difference?

I tried running the pystone benchmark, but the results were inconclusive. I saw very large differences (up to 20%) between runs at different times, with no clear differences with or without the patch.
However, a quick search shows that the operator module is almost completely unused by the rest of the code and the stdlib. So I really don't think performance is an issue here.

Attached a slightly revised patch thanks to Serhiy's review.
In addition to Serhiy's remarks, I used "_operator._compare_digest = _operator.eq" to reduce a bit more boilerplate.

The operator module is rarely used in the stdlib, but if it is used in user code (mainly with map(), reduce() or like) the performance often is important. You can use microbenchmarks like following (operator.add is twice faster than lambda x, y: x + y).
 ./python -m timeit -s "import operator; a = list(range(10000)); b = a[:]" -- "list(map(operator.add, a, b))"

I just ran such microbenchmarks for operator.add and operator.not_, and saw no significant difference with or without the patch.

Here's another complete conversion patch for _operator.
As suggested by Serhiy, I changed the comparison operators to copy the signature from _operator.eq() instead of _operator.lt(), which is easier to understand.

Here's another version of the _operator patch, with another small change after Serhiy's latest review.

issue20186._operator.v4.patch LGTM.

Attached is a revised patch for Modules/mathmodule.c incorporating changes suggested by Serhiy in his review.
The major change is the reformatting of the doc-strings of most of the functions in the module to use the same format as doc-strings generated by AC.

Summary of the work here as I pass through :)
Files already done:
 Objects/memoryobject.c: converted to signatures without Arg Clinic (Py 3.5)
 Modules/_opcode.c: Issue 19674 (3.4). Only one function I can see there (Larry’s original post says two sites).
 Modules/_io/_iomodule.c: part of Georg’s modules_issue20186.patch, but already handled in Issue 20175 (3.5)
Files being worked on:
 Modules/_operator.c: patch by Tal, ready for commit?
 Modules/_lsprof.c: Georg’s modules_issue20186.patch
 Modules/_heapqmodule.c: restored Georg’s patch; looks like he meant to add it to the main patch but it got lost
 Objects/enumobject.c: patch by Tal
 Modules/symtablemodule.c: Georg’s modules_issue20186.patch
 Modules/mathmodule.c: patch by Tal
 Modules/_tracemalloc.c: modules_issue20186.patch; needs update from review comments
 Modules/_csv.c: modules_issue20186.patch; couple of review comments
That leaves the following files from OP:
 Objects/tupleobject.c: 2 sites
 Objects/descrobject.c: 2 sites
 Objects/complexobject.c: 2 sites
 Objects/weakrefobject.c: 1 sites
 Objects/structseq.c: 1 sites
 Objects/rangeobject.c: 1 sites
 Objects/object.c: 1 sites
 Objects/moduleobject.c: 1 sites
 Objects/funcobject.c: 1 sites
 Objects/fileobject.c: 1 sites
 Objects/codeobject.c: 1 sites
 Objects/boolobject.c: 1 sites

> Modules/_opcode.c: Issue 19674 (3.4). Only one function I can see there (Larry’s original post says two sites).
I produced the post with a big grep through the codebase. Which was quite a while ago now. Code changes and moves around; if you can only find one site in the current file I believe you.

My patches haven't been looked at for a while, anyone considering them should make sure they still apply cleanly and don't break anything. Here their status as far as I can tell:
Quite a bit of work went into Modules/_operator.c, and it got an "LGTM" from Serhiy, so it should probably go in after a quick check.
Modules/mathmodule.c was also in a good state when I left it, and it underwent some review from Serhiy whose comments were implemented. It should probably be good to go after a thorough review.
Objects/enumobject.c is simpler but requires review.

New changeset 3713f7de576d by Serhiy Storchaka in branch 'default':
Issue #20186: Converted the _operator module to Argument Clinic.
https://hg.python.org/cpython/rev/3713f7de576d 

The only change I made is used the return converter in length_hint().

New changeset 112f27b8c8ea by Serhiy Storchaka in branch 'default':
Issue #20186: Converted the math module to Argument Clinic.
https://hg.python.org/cpython/rev/112f27b8c8ea 

issue20186.mathmodule.v2.patch needed just synchronizing docstrings.

issue20186.enumobject.patch LGTM too.

New changeset e3db9bccff3f by Serhiy Storchaka in branch 'default':
Issue #20186: Converted builtins enumerate() and reversed() to Argument Clinic.
https://hg.python.org/cpython/rev/e3db9bccff3f 

Sorry for committing patches for you Tal, but they were hanging so long time.

Serhiy, no apology is required. On the contrary, thank you for the taking the time to review this and commit, I don't have time available for this these days.

There are patches based on modules_issue20186.patch that convert to Argument 
Clinic 4 modules: _csv, _lsprof, _tracemalloc and symtable. They are 
synchronized with current sources and updated to current Argument Clinic. 
Other changes:
* Addressed Larry's comments.
* Used converter names instead of format units (object instead of 'O' etc).
* Removed unneeded self declarations in _lsprof.
* Removed changes for _csv.Dialect.__new__. Generated signature has None as 
default values, but this is not true. Actually parameters don't have default 
values. This function can't be converted.
* Parameters of _lsprof.Profiler.enable should have the int converter, not 
bool. Default values are -1 (meaning "don't change current value"), not True.
* Made the parameter of _tracemalloc._get_object_traceback() positional-only.
* Simplified code for creating a result in _traceback functions.

The application of AC to enumerate() lost information about the start argument and the signature of the call. We're going backwards.
---- New help -----------------------------------------------------
class enumerate(object)
 | Return an enumerate object.
 |
 | iterable
 | an object supporting iteration
 |
 | The enumerate object yields pairs containing a count (from start, which
 | defaults to zero) and a value yielded by the iterable argument.
 |
 | enumerate is useful for obtaining an indexed list:
 | (0, seq[0]), (1, seq[1]), (2, seq[2]), ...
---- Old help -----------------------------------------------------
class enumerate(object)
 | enumerate(iterable[, start]) -> iterator for index, value of iterable
 | 
 | Return an enumerate object. iterable must be another object that supports
 | iteration. The enumerate object yields pairs containing a count (from
 | start, which defaults to zero) and a value yielded by the iterable argument.
 | enumerate is useful for obtaining an indexed list:
 | (0, seq[0]), (1, seq[1]), (2, seq[2]), ...
Also, reversed() lost the indication of its signature:
 reversed(sequence) -> reverse iterator over values of the sequence
And the help doesn't have the usual:
 iterable
 | an object supporting iteration

When reviewing AC patches, we should always compare the help() before and after. 
Also, if the code already had fast parsing like:
 if (!PyArg_UnpackTuple(args, "reversed", 1, 1, &seq) )
there needs to be a before and after timing to make sure there wasn't a performance regression.
When PyArg_NoKeywords was present, we need to verify that the AC version also precludes keyword arguments (to prevent the creation of unhelpful keyword args and to keep compatibility with other versions of Python).

Microbenchmarks:
$ ./python -m perf timeit --duplicate 100 "enumerate('abc')"
Unpatched: Median +- std dev: 1.76 us +- 0.10 us
Patched: Median +- std dev: 1.61 us +- 0.07 us
$ ./python -m perf timeit --duplicate 100 "enumerate('abc', 1)"
Unpatched: Median +- std dev: 2.14 us +- 0.09 us
Patched: Median +- std dev: 1.76 us +- 0.07 us
$ ./python -m perf timeit --duplicate 100 "reversed('abc')"
Unpatched: Median +- std dev: 1.20 us +- 0.06 us
Patched: Median +- std dev: 1.20 us +- 0.07 us
enumerate() is 9-21% faster (due to avoiding of tuple creating), reversed() is not changed (Argument Clinic generates the same parsing code for it).

Here is a patch that restores ol docstrings for enumerate() and reversed(). But it may be better to change pydoc so that it would output a text signature of class constructor if available.

Argument Clinic generates incorrect parsing code for _csv.field_size_limit().

The problem with lsprof_clinic.patch is that it exposes default value of _lsprof.Profiler.enable() parameters as -1. Actually _lsprof.Profiler.enable() should accept boolean arguments without default value.

New changeset 7f8a3eb3459e by Serhiy Storchaka in branch 'default':
Issue #20186: Converted the symtable module to Argument Clinic.
https://hg.python.org/cpython/rev/7f8a3eb3459e 

New changeset e1df73b46094 by Serhiy Storchaka in branch 'default':
Issue #20186: Converted the tracemalloc module to Argument Clinic.
https://hg.python.org/cpython/rev/e1df73b46094 

New changeset b0ef37ec83f337b4b77275b367288a5656a0682c by Serhiy Storchaka in branch 'master':
Issue #20186: Converted the symtable module to Argument Clinic.
https://github.com/python/cpython/commit/b0ef37ec83f337b4b77275b367288a5656a0682c
New changeset 18a02e9d1f8e981b7b2f4287a4ed871021b13ade by Serhiy Storchaka in branch 'master':
Issue #20186: Converted the tracemalloc module to Argument Clinic.
https://github.com/python/cpython/commit/18a02e9d1f8e981b7b2f4287a4ed871021b13ade

New changeset 8ccb3ad39ee4 by Serhiy Storchaka in branch 'default':
Issue #20186: Regenerated Argument Clinic.
https://hg.python.org/cpython/rev/8ccb3ad39ee4 

New changeset 5762cf299f863e06244e6b44ba3a91efee7b35c1 by Serhiy Storchaka in branch 'master':
Issue #20186: Regenerated Argument Clinic.
https://github.com/python/cpython/commit/5762cf299f863e06244e6b44ba3a91efee7b35c1

PR 614: Objects/tupleobject.c 

New changeset 0b5615926a573c19c887a701a2f7047f4fd06de6 by Serhiy Storchaka in branch 'master':
bpo-20186: Convert tuple object implementation to Argument Clinic. (#614)
https://github.com/python/cpython/commit/0b5615926a573c19c887a701a2f7047f4fd06de6

I suspect change in this issue led to issue43413.