|
|
|
Created:
17 years, 4 months ago by Antoine Pitrou Modified:
5 years, 8 months ago Base URL:
http://svn.python.org/view/*checkout*/python/branches/py3k/ Visibility:
Public. |
Patch Set 1 #Patch Set 2 : New patch after cosmetic changes suggested by Benjamin & GvR #Patch Set 3 : Oops, right patch this time #Patch Set 4 : Fix some small exception bugs #
Total messages: 10
|
Benjamin
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 ...
|
17 years, 4 months ago (2008年08月19日 19:59:51 UTC) #1 | ||||||||||||||||||||||||||||
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.
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.
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.
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.
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.