classification
Title: bytearray_getbuffer: unnecessary code
Type: resource usage Stage: patch review
Components: Interpreter Core Versions: Python 3.3
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: ncoghlan, pitrou, skrah
Priority: normal Keywords: needs review, patch

Created on 2012-03-05 18:32 by skrah, last changed 2012-03-05 19:25 by skrah.

Files
File name Uploaded Description Edit
bytearray_getbuffer.diff skrah, 2012-03-05 18:32 review
Messages (3)
msg154969 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-03-05 18:32
bytearray_getbuffer() checks for view==NULL. But this has never been
allowed:

PEP: "The second argument is the address to a bufferinfo structure.
      Both arguments must never be NULL."

DOCS (3.2): "view must point to an existing Py_buffer structure
             allocated by the caller".


A quick grep through the source tree shows no instances where
the middle argument of either PyObject_GetBuffer of bf_getbuffer
is NULL or 0.


Patch attached, all tests pass. I wouldn't be comfortable to
commit it without review though (it's just too strange).


BTW, the next conditional in bytearray_getbuffer ...

    if (ret >= 0) {
        obj->ob_exports++;
    }

is also superfluous, since PyBuffer_FillInfo() cannot fail
if readonly==0.
msg154971 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-03-05 18:56
array_buffer_getbuf does a similar thing:

if (view==NULL) goto finish;

 finish:
    self->ob_exports++;   // ???
    return 0


Where does this view==NULL thing come from?
msg154972 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-03-05 19:25
I seems to be a feature to get a "lock" on an exporter
without the exporter filling in a buffer. It was there
from the beginning:

http://svn.python.org/view/python/branches/release30-maint/Objects/bytearrayobject.c?r1=56849&r2=57181


Last use that I can see is here:

http://hg.python.org/cpython/file/df3b2b5db900/Modules/posixmodule.c#l561
History
Date User Action Args
2012-03-06 16:39:42skrahlinkissue13860 superseder
2012-03-06 16:39:09skrahunlinkissue13860 dependencies
2012-03-05 19:25:31skrahsetmessages: + msg154972
2012-03-05 19:24:52skrahlinkissue13860 dependencies
2012-03-05 18:56:14skrahsetmessages: + msg154971
2012-03-05 18:32:48skrahcreate