Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(69)

#20152: Derby: Convert the _imp module to use Argument Clinic

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 1 month ago by brett
Modified:
5 years, 3 months ago
Reviewers:
larry, martin, storchaka
CC:
loewis, brett.cannon, larry, devnull_psf.upfronthosting.co.za, storchaka
Visibility:
Public.

Patch Set 1 #

Total comments: 5

Patch Set 2 #

Patch Set 3 #

Total comments: 2

Patch Set 4 #

Total comments: 1

Patch Set 5 #

Patch Set 6 #

Total comments: 3

Patch Set 7 #

Total comments: 3

Patch Set 8 #

Patch Set 9 #

Patch Set 10 #

Total comments: 1

Patch Set 11 #

Patch Set 12 #

Total comments: 5

Patch Set 13 #

Patch Set 14 #

Total comments: 19
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Modules/clinic/fcntlmodule.c.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +188 lines, -0 lines 0 comments Download
Modules/fcntlmodule.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 7 chunks +233 lines, -207 lines 19 comments Download

Messages

Total messages: 12
larry
Looks pretty good! Just some minor feedback. http://bugs.python.org/review/20152/diff/10485/Python/import.c File Python/import.c (right): http://bugs.python.org/review/20152/diff/10485/Python/import.c#newcode42 Python/import.c:42: format_unit = ...
6 years, 1 month ago #1
brett.cannon
http://bugs.python.org/review/20152/diff/10485/Python/import.c File Python/import.c (right): http://bugs.python.org/review/20152/diff/10485/Python/import.c#newcode42 Python/import.c:42: format_unit = 'O&' Hold-over from when I tried to ...
6 years, 1 month ago #2
larry
> > code: object(type="PyCodeObject *", subclass_of="&PyCode_Type") > > Done. You didn't remove the cast to ...
6 years, 1 month ago #3
loewis
http://bugs.python.org/review/20152/diff/10606/Modules/grpmodule.c File Modules/grpmodule.c (right): http://bugs.python.org/review/20152/diff/10606/Modules/grpmodule.c#newcode87 Modules/grpmodule.c:87: id: object You should probably use the positional-only syntax ...
5 years, 6 months ago #4
loewis
http://bugs.python.org/review/20152/diff/10608/Modules/spwdmodule.c File Modules/spwdmodule.c (right): http://bugs.python.org/review/20152/diff/10608/Modules/spwdmodule.c#newcode233 Modules/spwdmodule.c:233: This needs /*[clinic input] dump buffer [clinic start generated ...
5 years, 6 months ago #5
loewis
http://bugs.python.org/review/20152/diff/10610/Modules/pwdmodule.c File Modules/pwdmodule.c (right): http://bugs.python.org/review/20152/diff/10610/Modules/pwdmodule.c#newcode98 Modules/pwdmodule.c:98: uidobj: object It would be better if the parameter ...
5 years, 6 months ago #6
loewis
http://bugs.python.org/review/20152/diff/10668/Modules/fcntlmodule.c File Modules/fcntlmodule.c (right): http://bugs.python.org/review/20152/diff/10668/Modules/fcntlmodule.c#newcode39 Modules/fcntlmodule.c:39: code: int The parameter name should remain "op", in ...
5 years, 6 months ago #7
loewis
The patch is fine (with the typos corrected). Please make sure you rerun AC before ...
5 years, 6 months ago #8
loewis
With the cosmetic changes, the patch is fine, please apply. Make sure you run AC ...
5 years, 6 months ago #9
storchaka_gmail.com
http://bugs.python.org/review/20152/diff/13217/Modules/fcntlmodule.c File Modules/fcntlmodule.c (right): http://bugs.python.org/review/20152/diff/13217/Modules/fcntlmodule.c#newcode42 Modules/fcntlmodule.c:42: code: int In fcntl manpage this argument is named ...
5 years, 3 months ago #10
brett.cannon
Thanks for the review! I don't have time to do the code changes for the ...
5 years, 3 months ago #11
brett.cannon
5 years, 3 months ago #12
I'm going to open a separate issue for the doc changes Serhiy proposed.

http://bugs.python.org/review/20152/diff/13217/Modules/fcntlmodule.c
File Modules/fcntlmodule.c (right):

http://bugs.python.org/review/20152/diff/13217/Modules/fcntlmodule.c#newcode76
Modules/fcntlmodule.c:76: if (PyArg_ParseTuple(optional_arg_tuple, "s#", &str,
&len)) {
On 2014/11/07 20:30:43, storchaka wrote:
> PyArg_Parse(arg, "s#", &str, &len)
> 
> And same below.
> 
> Interesting, but string argument (unlike to int and bytes) is not used in the
> stdlib and is not tested. The argument should be either C int or pointer to
some
> C structure (corresponding to Python bytes). UTF-8 encoded string doesn't make
> sense here. This is remnants of Python 2 and should be eliminated.

Done.

http://bugs.python.org/review/20152/diff/13217/Modules/fcntlmodule.c#newcode107
Modules/fcntlmodule.c:107: ret = fcntl(fd, code, long_arg);
On 2014/11/07 20:30:43, storchaka wrote:
> Here is a bug. An argument should be int.

I take it this is a pre-existing bug? I don't see the issue with the code
conversion.

http://bugs.python.org/review/20152/diff/13217/Modules/fcntlmodule.c#newcode123
Modules/fcntlmodule.c:123: mutate_flag as mutate_arg: int = 1
On 2014/11/07 20:30:43, storchaka wrote:
> Actually this is boolean flag.

Done.

http://bugs.python.org/review/20152/diff/13217/Modules/fcntlmodule.c#newcode262
Modules/fcntlmodule.c:262: "I;ioctl requires a file or file descriptor,"
On 2014/11/07 20:30:43, storchaka wrote:
> Why "i" was changed to "I" here?

Accident. Fixed.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+