classification
Title: Derby #13: Convert 50 sites to Argument Clinic across 5 files
Type: enhancement Stage: patch review
Components: Argument Clinic, Extension Modules Versions: Python 3.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: georg.brandl, larry, loewis, nadeem.vawda, python-dev, serhiy.storchaka, taleinat, vstinner
Priority: normal Keywords: patch

Created on 2014-01-08 00:17 by larry, last changed 2018-09-12 07:35 by taleinat.

Files
File name Uploaded Description Edit
modules_issue20182.patch georg.brandl, 2014-01-13 07:13 review
modules_issue20182_v2.patch georg.brandl, 2014-01-16 08:29 review
modules_issue20182_v3.patch taleinat, 2015-04-24 00:00 updated version of Georg's patch, now applies cleanly to default branch with tests passing review
issue20182.signalmodule.patch taleinat, 2015-04-24 12:24 updated patch for signalmodule after Serhiy's review review
issue20182.selectmodule.patch taleinat, 2015-04-24 22:55 updated patch for selectmodule after Serhiy's review review
issue20182.selectmodule.v2.patch taleinat, 2015-04-25 07:44 updated patch; set epoll method function names back to what they were review
issue20182.sysmodule.patch taleinat, 2015-05-16 09:22 updated patch for sysmodule after Serhiy's review review
issue20182._hashopenssl.patch taleinat, 2015-05-16 11:04 AC conversion of Modules/_hashopenssl.c review
Pull Requests
URL Status Linked Edit
PR 4265 merged taleinat, 2017-11-03 21:58
PR 9213 open taleinat, 2018-09-12 07:34
Messages (38)
msg207639 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-08 00:17
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
msg207941 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-01-12 10:11
Here's the signalmodule.
msg207945 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-01-12 10:27
Now without crap.
msg207951 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-01-12 11:18
Here's sys.

For it to generate the correct code you need to add "args" to c_keywords in clinic.py.
msg208010 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-01-13 07:13
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.
msg208258 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-01-16 08:30
New patch addressing comments.
msg224138 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-07-27 15:41
The patch does not apply anymore. Can you please update it, and rerun AC on it?
msg224763 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-08-04 20:14
All the Derby patches should only go into trunk at this point.
msg241864 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-04-23 13:22
Note: Despite not appearing in any of these patches, the zlib module seems to have already been converted.
msg241868 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-04-23 16:45
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?
msg241872 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-23 18:19
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.
msg241900 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-04-24 00:00
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.
msg241916 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-24 06:36
It would be more helpful to split the patch on three independent patches for three modules.
msg241917 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-24 07:06
And it would be more helpful to use self-descriptive converter names instead of cryptic format units.
msg241919 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-04-24 07:32
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.
msg241922 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-24 07:51
I already have reviewed changes to select and signal.
msg241936 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-04-24 12:24
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.
msg241989 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-04-24 22:55
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"?
msg242003 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-04-25 07:44
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.
msg242476 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-03 14:25
Added new comments. issue20182.signalmodule.patch LGTM.
msg242488 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-03 15:48
Do you mean the v2.patch file, or the one before it?
msg242489 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-03 15:56
I don't see the v2 patch for the signal module.
msg242492 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-03 16:32
Sorry, my mistake, got mixed up with selectmodule.
msg243302 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-16 09:22
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.
msg243305 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-16 10:26
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.
msg243308 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-16 11:04
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".
msg243309 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-05-16 11:15
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
msg243310 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-16 11:40
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?
msg243312 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-05-16 12:58
New changeset 738ac3ad9ee4 by Serhiy Storchaka in branch 'default':
Fixed compilation error in signalmodule.c (issue #20182).
https://hg.python.org/cpython/rev/738ac3ad9ee4
msg304907 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-24 12:45
Tal, do you mind to update your patches and create pull requests?
msg304932 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2017-10-24 17:37
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.
msg305168 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2017-10-28 21:04
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?
msg305410 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2017-11-02 10:16
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!
msg305429 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2017-11-02 15:30
See issue #31926 and PR 4230 regarding the aforementioned argument clinic bug and a fix for it.
msg305483 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-11-03 13:49
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?
msg305484 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2017-11-03 13:54
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 :)
msg305519 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2017-11-03 21:59
See issue #31938 regarding Modules/selectmodule.c.
msg325127 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-09-12 07:35
See new updated PR9213 for completing the _hashopenssl.c conversion.
History
Date User Action Args
2018-09-12 07:35:53taleinatsetmessages: + msg325127
2018-09-12 07:34:42taleinatsetpull_requests: + pull_request8645
2017-11-03 21:59:53taleinatsetmessages: + msg305519
2017-11-03 21:58:15taleinatsetpull_requests: + pull_request4229
2017-11-03 13:54:28taleinatsetmessages: + msg305484
2017-11-03 13:49:53vstinnersetnosy: + vstinner
messages: + msg305483
2017-11-02 15:30:06taleinatsetmessages: + msg305429
2017-11-02 10:16:33taleinatsetmessages: + msg305410
2017-10-28 21:04:22taleinatsetmessages: + msg305168
2017-10-24 17:37:01taleinatsetmessages: + msg304932
2017-10-24 12:45:48serhiy.storchakasetmessages: + msg304907
2017-10-24 12:45:40serhiy.storchakasetmessages: - msg304906
2017-10-24 12:45:23serhiy.storchakasetmessages: + msg304906
2015-05-16 12:58:19python-devsetmessages: + msg243312
2015-05-16 11:40:23taleinatsetmessages: + msg243310
2015-05-16 11:15:00python-devsetnosy: + python-dev
messages: + msg243309
2015-05-16 11:04:50taleinatsetfiles: + issue20182._hashopenssl.patch

messages: + msg243308
2015-05-16 10:26:54serhiy.storchakasetmessages: + msg243305
2015-05-16 09:22:31taleinatsetfiles: + issue20182.sysmodule.patch

messages: + msg243302
2015-05-03 16:32:41taleinatsetmessages: + msg242492
2015-05-03 15:56:38serhiy.storchakasetmessages: + msg242489
2015-05-03 15:48:36taleinatsetmessages: + msg242488
2015-05-03 14:25:16serhiy.storchakasetmessages: + msg242476
2015-04-25 07:44:42taleinatsetfiles: + issue20182.selectmodule.v2.patch

messages: + msg242003
2015-04-24 22:55:37taleinatsetfiles: + issue20182.selectmodule.patch

messages: + msg241989
2015-04-24 12:24:57taleinatsetfiles: + issue20182.signalmodule.patch

messages: + msg241936
2015-04-24 07:51:33serhiy.storchakasetmessages: + msg241922
2015-04-24 07:32:51taleinatsetmessages: + msg241919
2015-04-24 07:06:38serhiy.storchakasetmessages: + msg241917
stage: needs patch -> patch review
2015-04-24 06:36:39serhiy.storchakasetmessages: + msg241916
2015-04-24 00:00:14taleinatsetfiles: + modules_issue20182_v3.patch

messages: + msg241900
2015-04-23 18:19:12serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg241872
2015-04-23 16:45:40taleinatsetmessages: + msg241868
2015-04-23 13:22:23taleinatsetnosy: + taleinat
messages: + msg241864
2015-02-25 15:28:23serhiy.storchakasetcomponents: + Argument Clinic
2014-08-04 20:14:36larrysetmessages: + msg224763
versions: + Python 3.5, - Python 3.4
2014-07-27 15:41:23loewissetnosy: + loewis
messages: + msg224138
2014-01-16 08:30:19georg.brandlsetmessages: + msg208258
2014-01-16 08:29:57georg.brandlsetfiles: + modules_issue20182_v2.patch
2014-01-13 07:13:11georg.brandlsetfiles: + modules_issue20182.patch

messages: + msg208010
2014-01-13 07:11:33georg.brandlsetfiles: - sys_clinic.patch
2014-01-13 07:11:30georg.brandlsetfiles: - signal_clinic.patch
2014-01-12 11:18:59georg.brandlsetfiles: + sys_clinic.patch

messages: + msg207951
2014-01-12 10:27:51georg.brandlsetfiles: + signal_clinic.patch

messages: + msg207945
2014-01-12 10:27:42georg.brandlsetfiles: - signal_clinic.patch
2014-01-12 10:11:49georg.brandlsetfiles: + signal_clinic.patch

nosy: + georg.brandl
messages: + msg207941

keywords: + patch
2014-01-08 11:32:41nadeem.vawdasetnosy: + nadeem.vawda
2014-01-08 01:36:37r.david.murraylinkissue20187 dependencies
2014-01-08 00:17:37larrycreate