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:
 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 

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.)

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.

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.)

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!

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.

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.

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.

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.)

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.

PC/winsound.c went pretty quick and easy.

Here's msvcrt.

And here is the patch to _winapi.c.

> 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).

Filed comments on everything.

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 :).

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.)

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.

> 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.

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 :)

Marking #20189 as a dependency; winreg.HKEYType and _winapi.Overlapped need the signature fixes provided by #20189 before commit.

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.

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.

Ok, v5 should be ready to go in, so long as you don't see anything scary.

I have high hopes for this latest update :)

2 hunks of _winapi.c currently fail to apply. Can you please update the patch?

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.

Here's an updated patch. It includes the patch from #20586 for proper signature/docstring output in _winapi.

Latest patch ought to be good, if anybody feels like rubber-stamping a 5000 line patch :)

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.

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.

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 

Committed, thanks for the reviews, guidance, and patience!

New changeset c8adc2c13c8b by Zachary Ware in branch 'default':
Issue #20172: Update clinicizations to current clinic.
https://hg.python.org/cpython/rev/c8adc2c13c8b