classification
Title: Derby #3: Convert 67 sites to Argument Clinic across 4 files (Windows)
Type: behavior Stage: resolved
Components: Argument Clinic, Extension Modules Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: 20189 20586 Superseder:
Assigned To: zach.ware Nosy List: larry, loewis, python-dev, steve.dower, zach.ware
Priority: normal Keywords: patch

Created on 2014-01-07 23:45 by larry, last changed 2015-05-13 15:58 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
conglomerate.v2.diff zach.ware, 2014-01-17 07:46 review
conglomerate.v3.diff zach.ware, 2014-01-22 03:35 review
conglomerate.v4-two-pass.diff zach.ware, 2014-01-23 16:16 review
conglomerate.v5-post-20189.diff zach.ware, 2014-01-25 06:24 review
conglomerate.v6-post-20326.diff zach.ware, 2014-01-28 15:04 review
issue20172.v7.diff zach.ware, 2014-08-04 19:49 review
issue20172.v8.diff zach.ware, 2015-04-14 20:07 review
Repositories containing patches
http://hg.python.org/sandbox/zware#issue20172
Messages (34)
msg207617 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-07 23:45
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:
    PC/msvcrtmodule.c: 18 sites
    PC/winreg.c: 24 sites
    PC/winsound.c: 3 sites
    Modules/_winapi.c: 22 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
msg207703 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-08 21:41
I'll take a stab at this one, but I may make you rue the day you said you'd review until your eyes bleed ;)

Here's a partial patch to PC/winreg.c, converting only the CloseKey function just to make sure I have some basic idea of what I'm doing.

(Also, if anyone else wants this one, please don't hesitate on account of me; just let me know and it's yours.)
msg207705 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-08 21:54
To possibly ease review (and for keeping track of what I'm doing), I'm linking hg.python.org/sandbox/zware#issue20172 where I'll try to do a commit per converted function.
msg207715 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-08 23:57
The piecemeal approach sounds fine, but I'm only going to review patches once you post them here.  (I'm not sure I can get to reviewing your patch today, but definitely tomorrow.)
msg207718 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-09 00:07
I lied, I just looked at it.  You said it was only one function, so it went quickly.

It looks totally fine.  In fact, Argument Clinic is generating better code than the original!
msg207719 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-09 00:08
Oh, and, p.s. I was a Win32 developer for about fifteen years.  I don't touch it anymore, but I consider myself still competent to read patches for simple stuff like the registry library.
msg207834 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-10 04:36
Thanks, Larry.  Conversion proceeds apace in winreg.c, but I have a couple questions.

1) Since the comment above CConverter.format_unit says all custom converters should use 'O&' instead of defining format_unit, there must be another way to do this:

/*[python input]
class REGSAM_converter(CConverter):
    type = 'REGSAM'
    format_unit = 'i'
    default = 0

    def converter_init(self, *, key_name=""):
        if key_name == "":
            raise ValueError('must provide the key name')
        self.doc_default = self.py_default = self.c_default = key_name
[python start generated code]*/

(see http://hg.python.org/sandbox/zware/rev/f0662bf33e65)

I don't know what the 'other way' is though :).  The above works, but I don't understand it completely and thus don't like it.  Also, it causes help(winreg.CreateKeyEx) to show "access='KEY_WRITE'" in the signature, when it should have no quotes (being a name).

2) Is there an easy way to give a function multiple names?  OpenKey and OpenKeyEx are the same function, OpenKeyEx has been just defined by an extra methoddef pointing at OpenKey; for now I've just modified that line to make things work.

Neither of these is blocking progress, so not a huge deal other than getting a little bit of review done early.
msg207854 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-10 11:48
1)
When I wrote that I hadn't considered that people would want custom subclasses of ints.  I assumed they'd be using custom converter *functions*, which of course means they'd use 'O&'.  I can think of how to reword the text but for now I assume your approach for REGSAM is fine; certainly I approve of using the correct type in the generated code.

However, I doubt doc_default, py_default, and c_default should all be exactly the same.  And the 'key_name' parameter seems a little awkward.

Here's something you could consider: I don't think it's documented yet (I'm going as fast as I can over here, honest) but now you can use simple constants as Python defaults.  So maybe you can use REGSAM like this:
    arg: REGSAM(c_default='KEY_READ') = winreg.KEY_READ

and then REGSAM could simply be an empty ("pass") subclass of int_converter.

2)
No convenient way yet.  Let me think about it.
msg207864 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-10 14:24
Ahhhh, much better.  An empty subclass doesn't do quite the right thing, but `class REGSAM_converter(int_converter): type = 'REGSAM'` does the trick (see http://hg.python.org/sandbox/zware/rev/5af08aa49631).

Thanks, Larry!

(Next will probably be trying to get a proper HKEY converter to work, but I'm holding off on that until I have most everything else clinicized first.)
msg207876 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-10 19:55
Here's the complete patch for PC/winreg.c.  One clinic/signature/pydoc issue I've noticed:

>>> help(winreg.HKEYType.Close)
Help on method_descriptor:

Close(...)                            <--- No signature
    Close()                           <--- Extra
    Closes the underlying Windows handle.

    If the handle is already closed, no error is raised.

>>> winreg.HKEYType.Close.__doc__
'Close()\nCloses the underlying Windows handle.\n\nIf the handle is already clos
ed, no error is raised.'
>>> winreg.HKEYType.Close.__text_signature__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'method_descriptor' object has no attribute '__text_signature__'


My gut feeling is that it's a Clinic issue; Clinic should be adding 'self' to the signature, which should then be picked up by the __text_signature__ parser, and used by inspect and pydoc.

As far as the patch, one point I'd like some extra scrutiny on is the HKEY_converter (and C clinic_HKEY_converter).  I don't understand how all of the C machinery there works properly, so I can't say with confidence that it is right.  It compiles without errors and the tests pass, but beyond that, I can't guarantee anything.
msg207881 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-10 20:35
PC/winsound.c went pretty quick and easy.
msg207898 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-11 07:09
Here's msvcrt.
msg208004 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-13 04:45
And here is the patch to _winapi.c.
msg208052 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-13 22:03
> Clinic should be adding 'self' to the signature,
> which should then be picked up by the __text_signature__
> parser, and used by inspect and pydoc.

This innocent little comment has derailed my whole day.  You're right, 'self' should be in the signature.  But not always!  And then in
inspect.Signature we need to strip it off for bound methods.

In case you're curious, this work is happening in a separate branch, and tracked in a different issue (#20189).
msg208064 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-14 01:51
Filed comments on everything.
msg208080 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-14 07:32
Sandbox repo updated.  It is currently using an older version of clinic; running current clinic on the winreg.c in the tip of the sandbox produces this traceback:

Traceback (most recent call last):
  File "Tools\clinic\clinic.py", line 2981, in <module>
    sys.exit(main(sys.argv[1:]))
  File "Tools\clinic\clinic.py", line 2977, in main
    parse_file(filename, output=ns.output, verify=not ns.force)
  File "Tools\clinic\clinic.py", line 1132, in parse_file
    cooked = clinic.parse(raw)
  File "Tools\clinic\clinic.py", line 1082, in parse
    parser.parse(block)
  File "Tools\clinic\clinic.py", line 2259, in parse
    self.state(line)
  File "Tools\clinic\clinic.py", line 2633, in state_parameter_docstring
    return self.next(self.state_parameter, line)
  File "Tools\clinic\clinic.py", line 2287, in next
    self.state(line)
  File "Tools\clinic\clinic.py", line 2531, in state_parameter
    value = eval(py_default)
  File "<string>", line 1, in <module>
NameError: name 'winreg' is not defined

I'm not terribly sure about the error handling with the return converters, that will need some extra scrutiny.  I may have it completely wrong :).
msg208149 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-15 06:38
Zachary: If you refresh your copy of trunk, you can now "clone" functions.  See the howto for the syntax.

The failure you were seeing was in some code that I just rewrote (see #20226).  I'd be interested if you could apply that patch and try your code again.  Or just wait maybe twelve hours, hopefully I'll have landed the patch by then.

(Yeah, it's kind of a moving target.  I'm trying to keep the pace up during the Derby.)
msg208198 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-15 21:50
About cloning:

Cloned functions still expect their own impl function.  In current winreg, OpenKey and OpenKeyEx literally are the same function by two names; is the best approach for this to define winreg_OpenKeyEx_impl as `return winreg_OpenKey_impl(module, key, sub_key, reserved, access);`?

As stated in #20226, that patch works fine with my conversions.  Once the 20226 patch lands, I'll get a comprehensive patch for this issue posted.  Until then, I've branched the sandbox repo yet again; future_clinic_20172 contains the 20226 patch, along with the necessary updates to these files and further changes using the new features that aren't in trunk yet.
msg208231 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-16 03:17
> is the best approach for this to define winreg_OpenKeyEx_impl
> as `return winreg_OpenKey_impl(module, key, sub_key, reserved,
> access);`?

Sounds good to me.
msg208742 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-22 03:47
Ok, new conglomerate patch is posted; includes a few more return conversions (particularly in msvcrt) and a few other minor changes compared to the previous conglomerate.  I would call this one a commit candidate, pending your review.  I don't plan to commit this one, though; I want to switch each of them to some manner of buffered output first, but I figured this form would be easier to review.  I'll post the buffered patch as soon as it's ready, you can review whichever one you actually prefer :)
msg208749 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-22 06:07
Marking #20189 as a dependency; winreg.HKEYType and _winapi.Overlapped need the signature fixes provided by #20189 before commit.
msg208750 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-22 06:18
And scratch v3 being commit-candidate.  I forgot to add an HKEY return converter to winreg (could have sworn I had done that, but I have no record of it), and my return converters in msvcrt are messy.
msg208969 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-23 16:46
Ok, here's a new patch.  This one fixes the issues I found in my last patch (HKEY return converter in winreg, mess from previous return converter attempts in msvcrt), and switches all four modules to a two-pass output scheme.  This should be basically what I want to commit, once 20189 is committed.

If you'd like a non-two-pass patch for review, let me know.
msg209156 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-25 06:26
Ok, v5 should be ready to go in, so long as you don't see anything scary.
msg209560 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-28 16:08
I have high hopes for this latest update :)
msg224118 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-07-27 11:20
2 hunks of _winapi.c currently fail to apply. Can you please update the patch?
msg224180 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-07-28 19:11
Certainly, Martin.  I'm in the process of getting it updated and self-reviewing again.  This patch will require #20586 and possibly another new Clinic feature (allowing output to multiple destinations; I have a change for that feature on my issue20172 sandbox branch, but don't remember why and never got an issue made for it) before commit.

I'll try to have a fresh patch up here within a week or so.
msg224752 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-08-04 19:49
Here's an updated patch.  It includes the patch from #20586 for proper signature/docstring output in _winapi.
msg241009 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-04-14 20:10
Latest patch ought to be good, if anybody feels like rubber-stamping a 5000 line patch :)
msg241117 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-04-15 15:28
Reviewed v8 and it looks good to me. Not a clinic expert, but I assume that it'll fail at build if anything is really wrong.
msg241130 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-04-15 16:37
If
  * the diff looks clean
  * it compiles without any *new* (sigh) errors, and
  * it passes the unit test suite without any *new* (sigh) failures,
then the Clinic conversion can generally be considered a success.
msg243044 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-05-13 06:32
New changeset d3582826d24c by Zachary Ware in branch 'default':
Issue #20172: Convert the winsound module to Argument Clinic.
https://hg.python.org/cpython/rev/d3582826d24c

New changeset 6e613ecd70f0 by Zachary Ware in branch 'default':
Issue #20172: Convert the winreg module to Argument Clinic.
https://hg.python.org/cpython/rev/6e613ecd70f0

New changeset c190cf615eb2 by Zachary Ware in branch 'default':
Issue #20172: Convert the msvcrt module to Argument Clinic.
https://hg.python.org/cpython/rev/c190cf615eb2

New changeset 4cf37a50d91a by Zachary Ware in branch 'default':
Issue #20172: Convert the _winapi module to Argument Clinic.
https://hg.python.org/cpython/rev/4cf37a50d91a
msg243045 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-05-13 06:34
Committed, thanks for the reviews, guidance, and patience!
msg243100 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-05-13 15:58
New changeset c8adc2c13c8b by Zachary Ware in branch 'default':
Issue #20172: Update clinicizations to current clinic.
https://hg.python.org/cpython/rev/c8adc2c13c8b
History
Date User Action Args
2015-05-13 15:58:54python-devsetmessages: + msg243100
2015-05-13 06:34:00zach.waresetstatus: open -> closed
messages: + msg243045

assignee: zach.ware
resolution: fixed
stage: needs patch -> resolved
2015-05-13 06:32:31python-devsetnosy: + python-dev
messages: + msg243044
2015-04-15 16:37:51larrysetmessages: + msg241130
2015-04-15 15:28:13steve.dowersetmessages: + msg241117
2015-04-14 20:10:19zach.waresetnosy: + steve.dower
messages: + msg241009
2015-04-14 20:07:14zach.waresetfiles: + issue20172.v8.diff
2015-02-25 15:26:50serhiy.storchakasetcomponents: + Argument Clinic
2014-08-04 19:49:20zach.waresetfiles: + issue20172.v7.diff

messages: + msg224752
versions: + Python 3.5, - Python 3.4
2014-07-28 19:11:25zach.waresetdependencies: + Argument Clinic: functions with valid sig but no docstring have no __text_signature__
messages: + msg224180
2014-07-27 11:20:52loewissetnosy: + loewis
messages: + msg224118
2014-01-28 16:08:29zach.waresetmessages: + msg209560
2014-01-28 15:04:55zach.waresetfiles: + conglomerate.v6-post-20326.diff
2014-01-28 15:04:38zach.waresetfiles: - conglomerate.v6-post-20326.diff
2014-01-28 14:55:14zach.waresetfiles: + conglomerate.v6-post-20326.diff
2014-01-25 06:26:53zach.waresetmessages: + msg209156
2014-01-25 06:25:14zach.waresetfiles: + conglomerate.v5-post-20189.diff
2014-01-23 16:46:39zach.waresetmessages: + msg208969
2014-01-23 16:16:43zach.waresetfiles: + conglomerate.v4-two-pass.diff
2014-01-22 06:18:13zach.waresetmessages: + msg208750
2014-01-22 06:07:30zach.waresetdependencies: + inspect.Signature doesn't recognize all builtin types
messages: + msg208749
2014-01-22 03:47:47zach.waresetmessages: + msg208742
2014-01-22 03:35:59zach.waresetfiles: + conglomerate.v3.diff
2014-01-17 07:50:07zach.waresetfiles: - issue20172._winapi.diff
2014-01-17 07:50:01zach.waresetfiles: - issue20172.msvcrt.diff
2014-01-17 07:49:54zach.waresetfiles: - issue20172.winsound.diff
2014-01-17 07:49:46zach.waresetfiles: - issue20172.winreg.diff
2014-01-17 07:46:47zach.waresetfiles: + conglomerate.v2.diff
2014-01-16 03:17:41larrysetmessages: + msg208231
2014-01-15 21:50:46zach.waresetmessages: + msg208198
2014-01-15 06:38:40larrysetmessages: + msg208149
2014-01-14 07:32:32zach.waresetmessages: + msg208080
2014-01-14 01:51:39larrysetmessages: + msg208064
2014-01-13 22:03:28larrysetmessages: + msg208052
2014-01-13 04:46:01zach.waresetfiles: + issue20172._winapi.diff

messages: + msg208004
2014-01-11 07:09:10zach.waresetfiles: + issue20172.msvcrt.diff

messages: + msg207898
2014-01-10 20:35:14zach.waresetfiles: + issue20172.winsound.diff

messages: + msg207881
2014-01-10 19:55:24zach.waresetmessages: + msg207876
2014-01-10 19:43:06zach.waresetfiles: - issue20172.partial.diff
2014-01-10 19:42:02zach.waresetfiles: + issue20172.winreg.diff
2014-01-10 14:24:29zach.waresetmessages: + msg207864
2014-01-10 11:48:48larrysetmessages: + msg207854
2014-01-10 04:36:19zach.waresetmessages: + msg207834
2014-01-09 00:08:30larrysetmessages: + msg207719
2014-01-09 00:07:23larrysetmessages: + msg207718
2014-01-08 23:57:40larrysetmessages: + msg207715
2014-01-08 21:54:33zach.waresethgrepos: + hgrepo216
messages: + msg207705
2014-01-08 21:41:11zach.waresetfiles: + issue20172.partial.diff

nosy: + zach.ware
messages: + msg207703

keywords: + patch
2014-01-08 01:36:37r.david.murraylinkissue20187 dependencies
2014-01-07 23:45:08larrycreate