(not quite finished) memoryview implementation

[Benjamin, 2008-08-19 19:59:51]

Looks pretty straightforward. Let GvR review though.e

http://codereview.appspot.com/3004/diff/1/4
File Lib/test/test_memoryview.py (right):

http://codereview.appspot.com/3004/diff/1/4#newcode11
Line 11: class CommonMemoryTests:
Since you name the tests explicitly in test_main, why don't you just make
CommonMemoryTests inherit from TestCase?

http://codereview.appspot.com/3004/diff/1/4#newcode18
Line 18: def _test_getitem_with_type(self, tp):
Instead of using _ in front of these, I think you should name them check_*. (ie
check_getitem_with_type)

http://codereview.appspot.com/3004/diff/1/4#newcode163
Line 163: def _view(self, obj):
Why not just make this an attribute?

http://codereview.appspot.com/3004/diff/1/4#newcode196
Line 196: class MemorySliceSliceTest(unittest.TestCase, CommonMemoryTests):
How are a slice and a slice of a slice different enough that they need separate
test cases?

http://codereview.appspot.com/3004/diff/1/2
File Objects/memoryobject.c (right):

http://codereview.appspot.com/3004/diff/1/2#newcode416
Line 416: "tolist() only accepts one-dimensional objects");
"accepts" should be "supports"

http://codereview.appspot.com/3004/diff/1/2#newcode787
Line 787: memory_richcompare,		        /* tp_richcompare */
Looks like /* tp_richcompare */ needs to be reindented.

[Antoine Pitrou, 2008-08-19 20:12:47]

http://codereview.appspot.com/3004/diff/1/4
File Lib/test/test_memoryview.py (right):

http://codereview.appspot.com/3004/diff/1/4#newcode11
Line 11: class CommonMemoryTests:
On 2008/08/19 19:59:52, Benjamin wrote:
> Since you name the tests explicitly in test_main, why don't you just make
> CommonMemoryTests inherit from TestCase?

Well it clearly signals that CommonMemoryTests is not a test case in itself.

http://codereview.appspot.com/3004/diff/1/4#newcode18
Line 18: def _test_getitem_with_type(self, tp):
On 2008/08/19 19:59:52, Benjamin wrote:
> Instead of using _ in front of these, I think you should name them check_*.
(ie
> check_getitem_with_type)

Ok, I'll do it.

http://codereview.appspot.com/3004/diff/1/4#newcode163
Line 163: def _view(self, obj):
On 2008/08/19 19:59:52, Benjamin wrote:
> Why not just make this an attribute?

Mmmh, because it's a method? :)
See other classes below.

http://codereview.appspot.com/3004/diff/1/4#newcode196
Line 196: class MemorySliceSliceTest(unittest.TestCase, CommonMemoryTests):
On 2008/08/19 19:59:52, Benjamin wrote:
> How are a slice and a slice of a slice different enough that they need
separate
> test cases?

There may be some edge cases, depending on the fact that the memoryview has a
"base" field or not, its format, shape etc. Better safe than sorry.

http://codereview.appspot.com/3004/diff/1/2
File Objects/memoryobject.c (right):

http://codereview.appspot.com/3004/diff/1/2#newcode416
Line 416: "tolist() only accepts one-dimensional objects");
On 2008/08/19 19:59:52, Benjamin wrote:
> "accepts" should be "supports"

Ok.

http://codereview.appspot.com/3004/diff/1/2#newcode787
Line 787: memory_richcompare,		        /* tp_richcompare */
On 2008/08/19 19:59:52, Benjamin wrote:
> Looks like /* tp_richcompare */ needs to be reindented.

Indeed.

[GvR, 2008-08-19 20:20:18]

Sorry, I can't review this. I don't know enough about the PEP 3118
implementation. :-(

http://codereview.appspot.com/3004/diff/1/2
File Objects/memoryobject.c (right):

http://codereview.appspot.com/3004/diff/1/2#newcode787
Line 787: memory_richcompare,		        /* tp_richcompare */
please use spaces here.

[Benjamin, 2008-08-19 21:19:23]

A few more comments. I think it's about ready to commit.

http://codereview.appspot.com/3004/diff/10/31
File Objects/memoryobject.c (right):

http://codereview.appspot.com/3004/diff/10/31#newcode606
Line 606: PyErr_SetNone(PyExc_NotImplementedError);
I think you want a "return NULL;" after this or it will fall through to the type
error. You also might as well add tests for these exceptions.

http://codereview.appspot.com/3004/diff/10/31#newcode621
Line 621: Py_buffer *view = &(self->view);
It's a bit confusing when you call Py_buffer variables "view" when there's
another struct called PyMemoryView.

[Antoine Pitrou, 2008-08-19 21:40:09]

http://codereview.appspot.com/3004/diff/10/31
File Objects/memoryobject.c (right):

http://codereview.appspot.com/3004/diff/10/31#newcode606
Line 606: PyErr_SetNone(PyExc_NotImplementedError);
On 2008/08/19 21:19:24, Benjamin wrote:
> I think you want a "return NULL;" after this or it will fall through to the
type
> error. You also might as well add tests for these exceptions.

Yes! Nice catch.

http://codereview.appspot.com/3004/diff/10/31#newcode621
Line 621: Py_buffer *view = &(self->view);
On 2008/08/19 21:19:24, Benjamin wrote:
> It's a bit confusing when you call Py_buffer variables "view" when there's
> another struct called PyMemoryView.

I know, but it's a convention used throughout this file. We can clean it up
later.

[tangyq_yoozoo.com, 2020-04-24 11:11:37]

http://localhost:8080/3004/

[tangyq_yoozoo.com, 2020-04-24 11:11:38]

http://localhost:8080/3004/

[tangyq_yoozoo.com, 2020-04-24 11:32:13]

http://localhost:8080/3004/

[tangyq_yoozoo.com, 2020-04-24 11:32:19]

http://localhost:8080/3004/

[tangyq_yoozoo.com, 2020-04-24 11:33:44]

http://localhost:8080/3004/