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

#10181: Problems with Py_buffer management in memoryobject.c (and elsewhere?)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 9 months ago by rupole
Modified:
6 years, 9 months ago
Reviewers:
pitrou, ncoghlan, stefan-usenet
CC:
loewis, oliphant_enthought.com, pmoore, mark.dickinson, Nick Coghlan, rupole_hotmail.com, AntoinePitrou, krisvale, haypo, josh_joshtriplett.org, pv, skrah, devnull_psf.upfronthosting.co.za, jcon, Petri Lehtinen
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 21

Patch Set 3 #

Total comments: 13

Patch Set 4 #

Patch Set 5 #

Patch Set 6 #

Patch Set 7 #

Total comments: 60

Patch Set 8 #

Total comments: 22

Patch Set 9 #

Total comments: 1

Patch Set 10 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/whatsnew/3.3.rst View 1 chunk +56 lines, -0 lines 0 comments Download
Misc/NEWS View 1 chunk +17 lines, -0 lines 0 comments Download

Messages

Total messages: 31
Nick Coghlan
That looks a *lot* cleaner than what we have now. See detailed comments for a ...
7 years, 5 months ago #1
stefan-usenet_bytereef.org
http://bugs.python.org/review/10181/diff/2978/7720 File Objects/memoryobject.c (right): http://bugs.python.org/review/10181/diff/2978/7720#newcode465 Objects/memoryobject.c:465: My comment was a bit cryptic. I meant that ...
7 years, 5 months ago #2
stefan-usenet_bytereef.org
http://bugs.python.org/review/10181/diff/2978/7717 File Include/memoryobject.h (right): http://bugs.python.org/review/10181/diff/2978/7717#newcode78 Include/memoryobject.h:78: #define Py_MEMORYVIEW_MAXSTATIC 3 Indeed. The constant is supposed to ...
7 years, 5 months ago #3
stefan-usenet_bytereef.org
http://bugs.python.org/review/10181/diff/2978/7718 File Include/object.h (left): http://bugs.python.org/review/10181/diff/2978/7718#oldcode161 Include/object.h:161: mono-dimensional buffers. */ Would it be OK to keep ...
7 years, 5 months ago #4
stefan-usenet_bytereef.org
http://bugs.python.org/review/10181/diff/2978/7720 File Objects/memoryobject.c (right): http://bugs.python.org/review/10181/diff/2978/7720#newcode142 Objects/memoryobject.c:142: mbuf = (PyManagedBufferObject *)v; Yes, this is nicer. I ...
7 years, 5 months ago #5
Nick Coghlan
http://bugs.python.org/review/10181/diff/2978/7717 File Include/memoryobject.h (right): http://bugs.python.org/review/10181/diff/2978/7717#newcode78 Include/memoryobject.h:78: #define Py_MEMORYVIEW_MAXSTATIC 3 Makes sense, although you may want ...
7 years, 5 months ago #6
pv
http://bugs.python.org/review/10181/diff/2978/7718 File Include/object.h (left): http://bugs.python.org/review/10181/diff/2978/7718#oldcode161 Include/object.h:161: mono-dimensional buffers. */ Numpy does not touch the `smalltable` ...
7 years, 5 months ago #7
stefan-usenet_bytereef.org
What's the etiquette for CC'ing? I don't want to leave anyone out, but perhaps the ...
7 years, 5 months ago #8
stefan-usenet_bytereef.org
http://bugs.python.org/review/10181/diff/2987/7744 File Objects/memoryobject.c (right): http://bugs.python.org/review/10181/diff/2987/7744#newcode257 Objects/memoryobject.c:257: mview->view = mbuf->master; I think cloning the arrays right ...
7 years, 5 months ago #9
Nick Coghlan
I think you already came to the same conclusion I did, but adding my comment ...
7 years, 5 months ago #10
AntoinePitrou
Hi, Looks good on the principle. I have an issue with the strategy adopted for ...
7 years, 5 months ago #11
Nick Coghlan
Since the "released" flag refers to whether or not ReleaseBuffer has been called, I think ...
7 years, 5 months ago #12
AntoinePitrou
http://bugs.python.org/review/10181/diff/2987/7744 File Objects/memoryobject.c (right): http://bugs.python.org/review/10181/diff/2987/7744#newcode83 Objects/memoryobject.c:83: PyErr_SetString(PyExc_BufferError, "several memoryviews are active"); On 2011/07/05 16:15:10, Nick ...
7 years, 5 months ago #13
stefan-usenet_bytereef.org
> Ok. However, the problem here is that several memoryviews share the same underlying state ...
7 years, 5 months ago #14
stefan-usenet_bytereef.org
7 years, 5 months ago #15
stefan-usenet_bytereef.org
http://bugs.python.org/review/10181/diff/2987/7744 File Objects/memoryobject.c (right): http://bugs.python.org/review/10181/diff/2987/7744#newcode61 Objects/memoryobject.c:61: _PyObject_GC_TRACK(mbuf); mbuf->master.obj is followed by tp_traverse() though. The docs ...
7 years, 5 months ago #16
AntoinePitrou
http://bugs.python.org/review/10181/diff/2987/7744 File Objects/memoryobject.c (right): http://bugs.python.org/review/10181/diff/2987/7744#newcode61 Objects/memoryobject.c:61: _PyObject_GC_TRACK(mbuf); On 2011/07/06 18:21:32, skrah wrote: > mbuf->master.obj is ...
7 years, 5 months ago #17
Nick Coghlan
Mostly just cosmetic comments on the docs and various comments, but there are two substantive ...
6 years, 11 months ago #18
Nick Coghlan
Mostly just cosmetic comments on the docs and various comments, but there are two substantive ...
6 years, 11 months ago #19
Nick Coghlan
Mostly just cosmetic comments on the docs and various comments, but there are two substantive ...
6 years, 11 months ago #20
Nick Coghlan
My apologies for the review spam - Reitveld was reporting a traceback, so I didn't ...
6 years, 11 months ago #21
stefan-usenet_bytereef.org
http://bugs.python.org/review/10181/diff/3343/10406 File Doc/c-api/buffer.rst (right): http://bugs.python.org/review/10181/diff/3343/10406#newcode134 Doc/c-api/buffer.rst:134: can use :c:member:`~Py_buffer.itemsize` to navigate the buffer. Fixed. http://bugs.python.org/review/10181/diff/3343/10406#newcode230 ...
6 years, 11 months ago #22
stefan-usenet_bytereef.org
I fixed the first batch of documentation issues.
6 years, 11 months ago #23
stefan-usenet_bytereef.org
http://bugs.python.org/review/10181/diff/3343/10406 File Doc/c-api/buffer.rst (right): http://bugs.python.org/review/10181/diff/3343/10406#newcode447 Doc/c-api/buffer.rst:447: I changed it to "order". I think it's best ...
6 years, 10 months ago #24
stefan-usenet_bytereef.org
I think I've addressed all of Nick's comments. In particular, tobytes() and PyMemoryView_GetContiguous() are fixed ...
6 years, 10 months ago #25
Nick Coghlan
Aside from a couple of very minor style nits and one questionable assert, the latest ...
6 years, 10 months ago #26
AntoinePitrou
http://bugs.python.org/review/10181/diff/4057/13587 File Doc/library/stdtypes.rst (right): http://bugs.python.org/review/10181/diff/4057/13587#newcode2467 Doc/library/stdtypes.rst:2467: >>> from _testbuffer import * Do we really want ...
6 years, 10 months ago #27
stefan-usenet_bytereef.org
Thanks for the additional review. I've added some answers and also two questions. http://bugs.python.org/review/10181/diff/4057/13586 File ...
6 years, 10 months ago #28
stefan-usenet_bytereef.org
http://bugs.python.org/review/10181/diff/4057/13589 File Include/memoryobject.h (right): http://bugs.python.org/review/10181/diff/4057/13589#newcode78 Include/memoryobject.h:78: } PyManagedBufferObject; Done in: http://hg.python.org/features/pep-3118/rev/6608872a2519 http://bugs.python.org/review/10181/diff/4057/13589#newcode81 Include/memoryobject.h:81: #define Py_MEMORYVIEW_MAX_NDIM ...
6 years, 10 months ago #29
stefan-usenet_bytereef.org
http://bugs.python.org/review/10181/diff/4057/13587 File Doc/library/stdtypes.rst (right): http://bugs.python.org/review/10181/diff/4057/13587#newcode2643 Doc/library/stdtypes.rst:2643: the array would use in a contiguous representation. It ...
6 years, 10 months ago #30
Nick Coghlan
6 years, 9 months ago #31
I didn't see anything significant this time around - I say go for it :)

http://bugs.python.org/review/10181/diff/4057/13586
File Doc/c-api/typeobj.rst (right):

http://bugs.python.org/review/10181/diff/4057/13586#newcode1263
Doc/c-api/typeobj.rst:1263: as the *view* argument.
I was the original proponent of the "address must stay the same" point of view,
but I was persuaded that keying off the internal pointer (as you now advise) is
the better approach.

http://bugs.python.org/review/10181/diff/4095/13888
File Doc/library/stdtypes.rst (right):

http://bugs.python.org/review/10181/diff/4095/13888#newcode2472
Doc/library/stdtypes.rst:2472: >>> m2.tolist()
Very minor style nit - you switched the later examples to x/y, but still use
m1/m2 here.
Sign in to reply to this message.

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