classification
Title: Correct __sizeof__ support for buffered I/O
Type: behavior Stage: resolved
Components: IO, Library (Lib) Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, hynek, jcea, loewis, meador.inge, pitrou, python-dev, serhiy.storchaka, stutzbach
Priority: normal Keywords: patch

Created on 2012-07-29 13:10 by serhiy.storchaka, last changed 2012-08-03 15:43 by jcea. This issue is now closed.

Files
File name Uploaded Description Edit
bufferedio_sizeof.patch serhiy.storchaka, 2012-07-29 13:56 review
bufferedio_sizeof-2.patch serhiy.storchaka, 2012-07-29 16:33 review
Messages (11)
msg166756 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-07-29 13:10
Here is a patch that implements correct __sizeof__ for C implementation of buffered I/O files: io.BufferedReader, io.BufferedWriter and io.BufferedRandom.
msg166758 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-07-29 13:31
There is no patch.
msg166769 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-29 14:44
Instead of putting this in MiscIOTest, you could use the relevant class-specific test cases.
msg166783 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2012-07-29 16:30
Serhiy, I didn't analyze it too in depth, but why aren't the test cases using the __sizeof__ support work you implemented for issue15467?  I think these tests should be using the more exact method like your other '__sizeof__' patches.
msg166784 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-07-29 16:33
> Instead of putting this in MiscIOTest, you could use the relevant
> class-specific test cases.

Thank you, good advice. Here is updated patch.
msg166786 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-07-29 16:50
> Serhiy, I didn't analyze it too in depth, but why aren't the test cases
> using the __sizeof__ support work you implemented for issue15467?  I think
> these tests should be using the more exact method like your other
> '__sizeof__' patches.

Because struct has not codes for Py_off_t and PyThread_type_lock.
msg166792 - (view) Author: Roundup Robot (python-dev) Date: 2012-07-29 17:07
New changeset 8a02a93c803a by Antoine Pitrou in branch '3.2':
Issue #15487: Add a __sizeof__ implementation for buffered I/O objects.
http://hg.python.org/cpython/rev/8a02a93c803a

New changeset 1d811e1097ed by Antoine Pitrou in branch 'default':
Issue #15487: Add a __sizeof__ implementation for buffered I/O objects.
http://hg.python.org/cpython/rev/1d811e1097ed
msg166794 - (view) Author: Roundup Robot (python-dev) Date: 2012-07-29 17:12
New changeset 1102e86b8739 by Antoine Pitrou in branch '2.7':
Issue #15487: Add a __sizeof__ implementation for buffered I/O objects.
http://hg.python.org/cpython/rev/1102e86b8739
msg166795 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-29 17:13
Thanks for the patch :)
msg166799 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2012-07-29 17:45
On Sun, Jul 29, 2012 at 11:50 AM, Serhiy Storchaka <report@bugs.python.org> wrote:

>> Serhiy, I didn't analyze it too in depth, but why aren't the test cases
>> using the __sizeof__ support work you implemented for issue15467?  I think
>> these tests should be using the more exact method like your other
>> '__sizeof__' patches.
>
> Because struct has not codes for Py_off_t and PyThread_type_lock.

Of course it doesn't -- those are Python specific typedefs.  'PyThread_type_lock'
is just a typedef to 'void *' and something could be figured out for 'Py_off_t'
in the support code.

Anyway, the way you are implementing the tests has the same issue as Martin pointed
out for the 'object.__sizeof__' method in issue15402.  I could replace the 
'buffered_sizeof' implementation with:

   static PyObject *
   buffered_sizeof(buffered *self, void *unused)
   {
       Py_ssize_t res;

       res = 1;
       if (self->buffer)
           res += self->buffer_size;
       return PyLong_FromSsize_t(res);
   }

and the tests will still pass.
msg166800 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-29 17:55
Le dimanche 29 juillet 2012 à 17:45 +0000, Meador Inge a écrit :
> Anyway, the way you are implementing the tests has the same issue as Martin pointed
> out for the 'object.__sizeof__' method in issue15402.  I could replace the 
> 'buffered_sizeof' implementation with:
> 
>    static PyObject *
>    buffered_sizeof(buffered *self, void *unused)
>    {
>        Py_ssize_t res;
> 
>        res = 1;
>        if (self->buffer)
>            res += self->buffer_size;
>        return PyLong_FromSsize_t(res);
>    }
> 
> and the tests will still pass.

That's true but, OTOH, I don't think it's very important. First, because
it is a rather unlikely mistake. Second, because the important thing is
to check that the internal buffer is part of the returned size, which is
what Serhiy's test checks for.
History
Date User Action Args
2012-08-03 15:43:05jceasetnosy: + jcea
2012-07-29 17:55:09pitrousetmessages: + msg166800
2012-07-29 17:45:17meador.ingesetnosy: + loewis
messages: + msg166799
2012-07-29 17:13:37pitrousetstatus: open -> closed
resolution: fixed
messages: + msg166795

stage: patch review -> resolved
2012-07-29 17:12:30python-devsetmessages: + msg166794
2012-07-29 17:07:33python-devsetnosy: + python-dev
messages: + msg166792
2012-07-29 16:50:53serhiy.storchakasetmessages: + msg166786
2012-07-29 16:33:44serhiy.storchakasetfiles: + bufferedio_sizeof-2.patch

messages: + msg166784
2012-07-29 16:30:54meador.ingesetnosy: + meador.inge

messages: + msg166783
stage: needs patch -> patch review
2012-07-29 14:44:29pitrousetmessages: + msg166769
2012-07-29 13:56:52serhiy.storchakasetfiles: + bufferedio_sizeof.patch
keywords: + patch
2012-07-29 13:31:27hyneksetmessages: + msg166758
stage: needs patch
2012-07-29 13:10:44serhiy.storchakacreate