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:
 Modules/signalmodule.c: 12 sites
 Modules/selectmodule.c: 12 sites
 Modules/zlibmodule.c: 11 sites
 Python/sysmodule.c: 10 sites
 Modules/_hashopenssl.c: 5 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 

Here's the signalmodule.

Now without crap.

Here's sys.
For it to generate the correct code you need to add "args" to c_keywords in clinic.py.

New patch.
In selectmodule.c I haven't touched devpoll and kqueue, which I can't compile here.
_hashopenssl does weird things with macros, I'm waiting with that.

New patch addressing comments.

The patch does not apply anymore. Can you please update it, and rerun AC on it?

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

Note: Despite not appearing in any of these patches, the zlib module seems to have already been converted.

I'm working on a new version of Georg's patch which applies to the current default branch.
I'm having trouble with AC output going into a separate file, since this requires a lot of things to be declared before the #include of the .c.h file. Should I move all of those definitions to the top of the file, even if they are currently spread all over it? Should I just add forward declarations? And if the latter, how do I forward-declare a struct typedef?

You only need include generated file before defining PyMethodDef arrays and PyTypeObject instances. There shouldn't be problems if only one class is defined in a file, otherwise you can move these static initializations together to the end of the file.

Here's a new version of Georg's patch which applies to the current default branch. On my OSX 10.8, CPython compiles fine and all tests pass, though test_devpoll and test_epoll were skipped.
I haven't made additional changes, so there is still AC-related work to be done here. For example, there are legacy converters used throughout the code, which should be replaced.
Changes I had to make to get this working:
1)
Updated the class declarations in selectmodule.c with the additional parameters.
2)
The doc-string for signal.sigwaitinfo was changed since Georg made his patch, and its first line was now too long. I shortened the first line from:
Wait synchronously for a signal until one of the signals in *sigset* is delivered.
to:
Wait synchronously until one of the signals in *sigset* is delivered.
3)
Converted the new sys.is_finalizing function.
4)
Reverted the signal.set_wakeup_fd function, since it has since been changed, and now depending on #ifdef MS_WINDOWS it interprets the argument as either 'O' or 'i'. I'm not sure what to do with this one, so I reverted it back to not use AC for now.
5)
I haven't converted the new signal.default_int_handler function, since it seems to accept any number of arguments, and ignores them. Is there a way to do this with AC?
6)
Moved the definitions of object structs and PyTypeObject types to the top of selectmodule.c, so that they are defined when Modules/clinic/selectmodule.c.h is imported.

It would be more helpful to split the patch on three independent patches for three modules.

And it would be more helpful to use self-descriptive converter names instead of cryptic format units.

Serhiy, I agree on both points. I can easily replace all of the converters and upload it as three separate patches. If you haven't started reviewing the patch yet, let me know and I'll do these things ASAP.

I already have reviewed changes to select and signal.

Here's a patch just for Modules/signalmodule.c (and related files), based on Georg's patch, updated to apply against current default, including fixes thanks to Serhiy's review.
This include one minor doc change, since I changed the name of the second parameter to signal.pthread_kill from "signum" to "signalnum" to be consistent with the rest of the signal module. This will not break existing code since this parameter is positional-only.

Here's a patch just for Modules/selectmodule.c (and related files), based on Georg's patch, updated to apply against current default, including fixes thanks to Serhiy's review.
It took a while since I had to get a Linux VM up to run the epoll tests. And it was good that I did, since Georg's patch had several errors in that area.
It is important to note that epoll.poll now raises a different exception when called with invalid arguments after it has been closed. It used to raise ValueError since it first checked whether it was closed before checking the parameters. With this patch, the parameters are checked first so a TypeError is raised. This behavior wasn't documented and wasn't tested, so it seems relatively safe to change. At Serhiy's suggestion, I added a test for the new behavior. Perhaps this should be mentioned in the release notes and/or "what's new"?

Here's a slightly modified version of the most recent patch for Modules/selectmodule.c. The only difference relative to the previous version is that I've set the epoll method function names back to what they used to be. My reasoning is to stay consistent with the other epoll method function names in the file and to keep the code familiar for the maintainers.

Added new comments. issue20182.signalmodule.patch LGTM.

Do you mean the v2.patch file, or the one before it?

I don't see the v2 patch for the signal module.

Sorry, my mistake, got mixed up with selectmodule.

Here's a patch just for Modules/sysmodule.c (and related files), based on Georg's patch, updated to apply against current default, including fixes thanks to Serhiy's review.
Despite Serhiy's suggestions, I've not used parameter groups, due to Larry's statement of his dislike of this (ab)use of the feature.
I've left an alias of the form "args as args_value:" since the latest clinic.py doesn't automatically alias "args" (causing a compile error).
At Serhiy's suggestion, I've also converted sys.callstats. I couldn't find any tests for it, so someone should definitely review this conversion!
All tests pass on my OSX 10.10.

The signature of sys.getsizeof() is not correct. sys.getsizeof(obj) is not the same as sys.getsizeof(obj, None). If not use optional group, then this function should not be converted to Argument Clinic. See also dict.get() with similar signature.
Opened new issue (issue24207) for the bug with the args parameter.
You can commit the patch for the signal module Tal.

Attached in a conversion patch for Modules/_hashopenssl.c.
Since it appear that zlib has already been converted, that's the last file to convert in this batch!
All tests pass on my OSX 10.10 (after running "touch Modules/hashlib.h" to get make to recompile things that use _hashopenssl).
Notes:
1)
In HASH.__init__, the "name" parameter must be a string. However, the current code accepts any object and checks something else before checking whether the given object is a string. Therefore, changing the type from "object" to "str" would slightly change the parameter checking behavior. For now I've left it as it was. Also, since HASH.__new__ mirrors this, I've left it as-is as well.
2)
I slightly changed the first line of the HASH object's doc-string, to make it fit in one line. It was: "A hash represents the object used to calculate a checksum of a string of information." I've changed the beginning to "A hash is an object used to calculate ..."
3)
pbkdb_hmac accepts an optional "dklen" parameter which may be either a long or None. I left this as an "object", since long(accept={int, NoneType}) gives "long_converter: default value None for field dklen_obj is not of type int".

New changeset 5342fad7cd59 by Tal Einat in branch 'default':
Issue #20182: converted the signal module to use Argument Clinic
https://hg.python.org/cpython/rev/5342fad7cd59 

Serhiy: Indeed, you're right about sys.getsizeof(). I'm tending towards changign it to use an optional group and per Serhiy's suggestion. Larry?

New changeset 738ac3ad9ee4 by Serhiy Storchaka in branch 'default':
Fixed compilation error in signalmodule.c (issue #20182).
https://hg.python.org/cpython/rev/738ac3ad9ee4 

Tal, do you mind to update your patches and create pull requests?

I'd be happy to update the patches.
I asked for a bit of clarification on what this entails in msg304931 on issue #20180, once that's clear I'll do the same for these patches and create PRs.

Regarding the select module, the existing patch moves typedefs and object type declarations to the top of the file with the #include clinic/selectmodule.c.h statement can come afterwards. Should I keep it this way, or instead move the method list and type definitions to the bottom of the file, as Serhiy said was preferable with itertoolsmodule.c?

I have a complete, up-to-date AC conversion of Modules/selectmodule.c ready on a branch in my GitHub CPython fork:
https://github.com/taleinat/cpython/tree/bpo-20182_AC_selectmodule
I haven't created a PR because there's currently an issue with the generated selectmodule.c.h missing a few "safety" #defines, causing compilation errors e.g. on Windows where poll is unavailable. Any help resolving this issue would be welcome!

See issue #31926 and PR 4230 regarding the aforementioned argument clinic bug and a fix for it.

I really hate the title of these issues "Derby #13: Convert 50 sites to Argument Clinic across 5 files". I would prefer to have one issue per file. Would it possible to close this one and open a new issue once someone has a PR for one file?

As the author of all of the updated patches, I wouldn't mind opening new issues separately for each of the remaining modules. Actually I would prefer it :)

See issue #31938 regarding Modules/selectmodule.c.

See new updated PR9213 for completing the _hashopenssl.c conversion.

New changeset c6c7237272499b2c528acb5f62601421f329e92a by Tal Einat in branch 'master':
bpo-20182: AC convert remaining functions/methods in _hashopenssl.c (GH-9213)
https://github.com/python/cpython/commit/c6c7237272499b2c528acb5f62601421f329e92a

The conversion of Modules/_hashopenssl.c leaves just Python/sysmodule.c to be converted from this batch.
There's an old patch here from 3.5 years ago that will likely need to be heavily updated.

See new PR GH-11328 with updated AC conversion of Python/sysmodule.c.

PR 9213 added a compiler warning:
In file included from /home/serhiy/py/cpython/Modules/_hashopenssl.c:69:0:
/home/serhiy/py/cpython/Modules/clinic/_hashopenssl.c.h:90:1: warning: ‘EVP_tp_init’ defined but not used [-Wunused-function]
 EVP_tp_init(PyObject *self, PyObject *args, PyObject *kwargs)
 ^~~~~~~~~~~
help(_hashlib.HASH) shows now the signature of the constructor of the object. But a hash object can not be created using it.
class HASH(builtins.object)
 | HASH(name, string=b'')
 | 
 | A hash is an object used to calculate a checksum of a string of information.
...

New changeset ede0b6fae20290bf22b6ee1b9a1e1179d750f360 by Tal Einat in branch 'master':
bpo-20182: AC convert Python/sysmodule.c (GH-11328)
https://github.com/python/cpython/commit/ede0b6fae20290bf22b6ee1b9a1e1179d750f360

Note that the problematic part of the Modules/_hashopenssl.c AC conversion was reverted, as part of GH-11379.

AFAICT, this issue can finally be closed!

Seems PR 11328 introduced a compiler warning:
In file included from ./Include/Python.h:64:0,
 from ./Python/sysmodule.c:17:
./Python/sysmodule.c:1597:14: warning: ‘sys_clear_type_cache__doc__’ defined but not used [-Wunused-variable]
 PyDoc_STRVAR(sys_clear_type_cache__doc__,
 ^
./Include/pymacro.h:70:37: note: in definition of macro ‘PyDoc_VAR’
 #define PyDoc_VAR(name) static char name[]
 ^~~~
./Python/sysmodule.c:1597:1: note: in expansion of macro ‘PyDoc_STRVAR’
 PyDoc_STRVAR(sys_clear_type_cache__doc__,
 ^~~~~~~~~~~~

Thanks Serhiy! The compiler doesn't warn about that on Windows.
See PR GH-11444 with a fix.