Author ncoghlan
Recipients gvanrossum, ncoghlan, pitrou
Date 2010-09-08.21:22:34
SpamBayes Score 2.52193e-12
Marked as misclassified No
Message-id <1283980970.56.0.201729797352.issue9757@psf.upfronthosting.co.za>
In-reply-to
Content
On an eyeball review, the structure of do_release seems a little questionable to me. I'd be happier if view.obj and view.buf were copied to C locals and then set to NULL at the start of the function before any real work is done.

I believe the buffer release process can trigger __del__ methods (and possibly other Python code), which may currently see the memoryview in a slightly inconsistent state when reference cycles are involved. Setting these two references to NULL early should keep that from happening (the idea of using C locals to avoid this is the same concept as is employed by the Py_CLEAR macro)

For the test code, I suggest including:
- a test where release() is called inside the cm to check that __exit__ does not raise ValueError.
- check getbuffer() correctly raises ValueError
- tests that the other properties correctly raise ValueError (ndim, shape, strides, format, itemsize, suboffsets)

(For the properties, perhaps using a list of strings, a loop and getattr rather than writing each one out separately)

_check_released should perhaps also take a second argument that is used for the last two NotEqual checks. That is:

def _check_released(self, m, tp):
  ...
  self.assertNotEqual(m, memoryview(tp()))
  self.assertNotEqual(m, tp())

And then pass in tp as well as m for the respective tests.

There's also a minor typo in a comment in the tests: "called at second" should be "called a second".

Other than that, looks good to me.
History
Date User Action Args
2010-09-08 21:22:50ncoghlansetrecipients: + ncoghlan, gvanrossum, pitrou
2010-09-08 21:22:50ncoghlansetmessageid: <1283980970.56.0.201729797352.issue9757@psf.upfronthosting.co.za>
2010-09-08 21:22:35ncoghlanlinkissue9757 messages
2010-09-08 21:22:34ncoghlancreate