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

#20193: Derby: Convert the zlib, _bz2 and _lzma modules to use Argument Clinic

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 8 months ago by storchaka+cpython
Modified:
5 years, 8 months ago
Reviewers:
nadeem.vawda, larry
CC:
larry, nadeem.vawda, meadori, devnull_psf.upfronthosting.co.za, storchaka
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Total comments: 2

Patch Set 5 #

Patch Set 6 #

Total comments: 3

Patch Set 7 #

Total comments: 1

Patch Set 8 #

Total comments: 2

Patch Set 9 #

Patch Set 10 #

Patch Set 11 #

Patch Set 12 #

Total comments: 7

Patch Set 13 #

Patch Set 14 #

Patch Set 15 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Modules/clinic/zlibmodule.c.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +411 lines, -0 lines 0 comments Download
Modules/zlibmodule.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 25 chunks +196 lines, -307 lines 0 comments Download

Messages

Total messages: 8
nadeem.vawda
http://bugs.python.org/review/20193/diff/10616/Modules/zlibmodule.c File Modules/zlibmodule.c (right): http://bugs.python.org/review/20193/diff/10616/Modules/zlibmodule.c#newcode586 Modules/zlibmodule.c:586: if ((size_t)pinput.len > UINT_MAX) { I think that this ...
5 years, 8 months ago #1
storchaka_gmail.com
http://bugs.python.org/review/20193/diff/10616/Modules/zlibmodule.c File Modules/zlibmodule.c (right): http://bugs.python.org/review/20193/diff/10616/Modules/zlibmodule.c#newcode586 Modules/zlibmodule.c:586: if ((size_t)pinput.len > UINT_MAX) { On 2014/01/19 18:22:30, nadeem.vawda ...
5 years, 8 months ago #2
nadeem.vawda
http://bugs.python.org/review/20193/diff/10618/Modules/_lzmamodule.c File Modules/_lzmamodule.c (right): http://bugs.python.org/review/20193/diff/10618/Modules/_lzmamodule.c#newcode494 Modules/_lzmamodule.c:494: " PyMem_Free(", name, ".options);\n"]) On 2014/01/19 18:37:33, storchaka wrote: ...
5 years, 8 months ago #3
nadeem.vawda
http://bugs.python.org/review/20193/diff/10688/Modules/_bz2module.c File Modules/_bz2module.c (right): http://bugs.python.org/review/20193/diff/10688/Modules/_bz2module.c#newcode299 Modules/_bz2module.c:299: Compression level, in 0-9. Rather: Compression level, as a ...
5 years, 8 months ago #4
storchaka_gmail.com
http://bugs.python.org/review/20193/diff/10688/Modules/_bz2module.c File Modules/_bz2module.c (right): http://bugs.python.org/review/20193/diff/10688/Modules/_bz2module.c#newcode299 Modules/_bz2module.c:299: Compression level, in 0-9. On 2014/01/22 21:55:08, nadeem.vawda wrote: ...
5 years, 8 months ago #5
larry
Only a review of one line, not the whole patch. http://bugs.python.org/review/20193/diff/10687/Modules/zlibmodule.c File Modules/zlibmodule.c (right): http://bugs.python.org/review/20193/diff/10687/Modules/zlibmodule.c#newcode274 ...
5 years, 8 months ago #6
larry
http://bugs.python.org/review/20193/diff/10754/Modules/zlibmodule.c File Modules/zlibmodule.c (right): http://bugs.python.org/review/20193/diff/10754/Modules/zlibmodule.c#newcode85 Modules/zlibmodule.c:85: output preset file Please don't do this to other ...
5 years, 8 months ago #7
nadeem.vawda
5 years, 8 months ago #8
http://bugs.python.org/review/20193/diff/10754/Modules/zlibmodule.c
File Modules/zlibmodule.c (right):

http://bugs.python.org/review/20193/diff/10754/Modules/zlibmodule.c#newcode85
Modules/zlibmodule.c:85: output preset file
On 2014/01/25 14:55:28, larry wrote:
> Please don't do this to other people's files.  I believe Nadeem Vawda owns
> _pickle.c.  If Nadeem gives you consent, you may add this line.

This is fine with me. (Though I think you mean zlibmodule.c -
I don't have any special knowledge of the _pickle module :P)

http://bugs.python.org/review/20193/diff/10754/Modules/zlibmodule.c#newcode283
Modules/zlibmodule.c:283: /*[-clinic input]
On 2014/01/25 14:55:28, larry wrote:
> If we add DEF_WBITS and DEFAULTALLOC constants to the module, we can convert
> this function now.  I'm willing to accept that change for 3.4.

DEF_WBITS is already present as MAX_WBITS; we can use that. (Maybe also get rid
of the DEF_WBITS macro; it isn't adding much value as an alias for MAX_WBITS...)

I'm fine with adding DEFAULTALLOC, but please rename it to something like
DEF_BUF_SIZE, for clarity and consistency with DEF_MEM_LEVEL. (Also, the comment
about buffer size growth on line 33 is inaccurate; maybe delete it?)
Sign in to reply to this message.

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