Nick Coghlan <ncogh...@gmail.com> added the comment: 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. ---------- _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue9757> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
