msg207617 - (view) |
Author: Larry Hastings (larry) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2014-01-10 20:35 |
PC/winsound.c went pretty quick and easy.
|
msg207898 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-01-11 07:09 |
Here's msvcrt.
|
msg208004 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-01-13 04:45 |
And here is the patch to _winapi.c.
|
msg208052 - (view) |
Author: Larry Hastings (larry) * |
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) * |
Date: 2014-01-14 01:51 |
Filed comments on everything.
|
msg208080 - (view) |
Author: Zachary Ware (zach.ware) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2014-01-28 16:08 |
I have high hopes for this latest update :)
|
msg224118 - (view) |
Author: Martin v. Löwis (loewis) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) |
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) * |
Date: 2015-05-13 06:34 |
Committed, thanks for the reviews, guidance, and patience!
|
msg243100 - (view) |
Author: Roundup Robot (python-dev) |
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
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:56 | admin | set | github: 64371 |
2015-05-13 15:58:54 | python-dev | set | messages:
+ msg243100 |
2015-05-13 06:34:00 | zach.ware | set | status: open -> closed messages:
+ msg243045
assignee: zach.ware resolution: fixed stage: needs patch -> resolved |
2015-05-13 06:32:31 | python-dev | set | nosy:
+ python-dev messages:
+ msg243044
|
2015-04-15 16:37:51 | larry | set | messages:
+ msg241130 |
2015-04-15 15:28:13 | steve.dower | set | messages:
+ msg241117 |
2015-04-14 20:10:19 | zach.ware | set | nosy:
+ steve.dower messages:
+ msg241009
|
2015-04-14 20:07:14 | zach.ware | set | files:
+ issue20172.v8.diff |
2015-02-25 15:26:50 | serhiy.storchaka | set | components:
+ Argument Clinic |
2014-08-04 19:49:20 | zach.ware | set | files:
+ issue20172.v7.diff
messages:
+ msg224752 versions:
+ Python 3.5, - Python 3.4 |
2014-07-28 19:11:25 | zach.ware | set | dependencies:
+ Argument Clinic: functions with valid sig but no docstring have no __text_signature__ messages:
+ msg224180 |
2014-07-27 11:20:52 | loewis | set | nosy:
+ loewis messages:
+ msg224118
|
2014-01-28 16:08:29 | zach.ware | set | messages:
+ msg209560 |
2014-01-28 15:04:55 | zach.ware | set | files:
+ conglomerate.v6-post-20326.diff |
2014-01-28 15:04:38 | zach.ware | set | files:
- conglomerate.v6-post-20326.diff |
2014-01-28 14:55:14 | zach.ware | set | files:
+ conglomerate.v6-post-20326.diff |
2014-01-25 06:26:53 | zach.ware | set | messages:
+ msg209156 |
2014-01-25 06:25:14 | zach.ware | set | files:
+ conglomerate.v5-post-20189.diff |
2014-01-23 16:46:39 | zach.ware | set | messages:
+ msg208969 |
2014-01-23 16:16:43 | zach.ware | set | files:
+ conglomerate.v4-two-pass.diff |
2014-01-22 06:18:13 | zach.ware | set | messages:
+ msg208750 |
2014-01-22 06:07:30 | zach.ware | set | dependencies:
+ inspect.Signature doesn't recognize all builtin types messages:
+ msg208749 |
2014-01-22 03:47:47 | zach.ware | set | messages:
+ msg208742 |
2014-01-22 03:35:59 | zach.ware | set | files:
+ conglomerate.v3.diff |
2014-01-17 07:50:07 | zach.ware | set | files:
- issue20172._winapi.diff |
2014-01-17 07:50:01 | zach.ware | set | files:
- issue20172.msvcrt.diff |
2014-01-17 07:49:54 | zach.ware | set | files:
- issue20172.winsound.diff |
2014-01-17 07:49:46 | zach.ware | set | files:
- issue20172.winreg.diff |
2014-01-17 07:46:47 | zach.ware | set | files:
+ conglomerate.v2.diff |
2014-01-16 03:17:41 | larry | set | messages:
+ msg208231 |
2014-01-15 21:50:46 | zach.ware | set | messages:
+ msg208198 |
2014-01-15 06:38:40 | larry | set | messages:
+ msg208149 |
2014-01-14 07:32:32 | zach.ware | set | messages:
+ msg208080 |
2014-01-14 01:51:39 | larry | set | messages:
+ msg208064 |
2014-01-13 22:03:28 | larry | set | messages:
+ msg208052 |
2014-01-13 04:46:01 | zach.ware | set | files:
+ issue20172._winapi.diff
messages:
+ msg208004 |
2014-01-11 07:09:10 | zach.ware | set | files:
+ issue20172.msvcrt.diff
messages:
+ msg207898 |
2014-01-10 20:35:14 | zach.ware | set | files:
+ issue20172.winsound.diff
messages:
+ msg207881 |
2014-01-10 19:55:24 | zach.ware | set | messages:
+ msg207876 |
2014-01-10 19:43:06 | zach.ware | set | files:
- issue20172.partial.diff |
2014-01-10 19:42:02 | zach.ware | set | files:
+ issue20172.winreg.diff |
2014-01-10 14:24:29 | zach.ware | set | messages:
+ msg207864 |
2014-01-10 11:48:48 | larry | set | messages:
+ msg207854 |
2014-01-10 04:36:19 | zach.ware | set | messages:
+ msg207834 |
2014-01-09 00:08:30 | larry | set | messages:
+ msg207719 |
2014-01-09 00:07:23 | larry | set | messages:
+ msg207718 |
2014-01-08 23:57:40 | larry | set | messages:
+ msg207715 |
2014-01-08 21:54:33 | zach.ware | set | hgrepos:
+ hgrepo216 messages:
+ msg207705 |
2014-01-08 21:41:11 | zach.ware | set | files:
+ issue20172.partial.diff
nosy:
+ zach.ware messages:
+ msg207703
keywords:
+ patch |
2014-01-08 01:36:37 | r.david.murray | link | issue20187 dependencies |
2014-01-07 23:45:08 | larry | create | |