Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(3579)

Issue 63153: Partial fix of #299

Can't Edit
Can't Publish+Mail
Start Review
Created:
14 years, 10 months ago by kwmsmith
Modified:
4 months, 2 weeks ago
Reviewers:
dagss, dag sverre seljebotn <dagss
CC:
kwmsmith
Visibility:
Public.

Patch Set 1 #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+125 lines, -79 lines) Patch
M Cython/Compiler/Buffer.py View 18 chunks +104 lines, -63 lines 8 comments Download
M Cython/Compiler/ExprNodes.py View 1 chunk +1 line, -3 lines 0 comments Download
M Cython/Compiler/Naming.py View 1 chunk +2 lines, -4 lines 1 comment Download
M Cython/Compiler/Nodes.py View 1 chunk +3 lines, -3 lines 0 comments Download
M Cython/Compiler/Options.py View 1 chunk +3 lines, -0 lines 0 comments Download
M Cython/Compiler/PyrexTypes.py View 1 chunk +9 lines, -0 lines 0 comments Download
M Cython/Compiler/Symtab.py View 1 chunk +3 lines, -6 lines 0 comments Download

Messages

Total messages: 2
kwmsmith
Hi, I'm trying out codereview.appspot.com for reviewing my gsoc patches. Please let me know if ...
14 years, 10 months ago (2009-05-27 16:27:58 UTC) #1
dagss
14 years, 10 months ago (2009-05-27 17:30:54 UTC) #2
Seems to work great! See comments.

http://codereview.appspot.com/63153/diff/1/2
File Cython/Compiler/Buffer.py (right):

http://codereview.appspot.com/63153/diff/1/2#newcode218
Line 218: def put_unpack_buffer_aux_into_scope(buf_entry, code):
Future improvement: The code generated here could be done statically within
__Pyx_GetBufferAndValidate instead.

In theory I think such a cleanup could harm performance (compiler could perhaps
no longer optimize away e.g. "suboffsets"), though I can't imagine it really
hurting anything measurable.

http://codereview.appspot.com/63153/diff/1/2#newcode224
Line 224: stobuf = [('stride', 'strides'), ('shape', 'shape')]
I suppose changing our Cython struct to match Py_buffer would simplify the code
and make things easier to remember.

http://codereview.appspot.com/63153/diff/1/2#newcode264
Line 264: return "__Pyx_SafeReleaseBuffer(&%s.pyxbuf->bufinfo)" %
entry.buffer_aux.pyx_buf_nd_var.cname
If __Pyx_GetBufferAndValidate is changed according to my other comments, then
the body of __Pyx_SafeReleaseBuffer should be modified instead so that it takes
"&%s" for consistency.

http://codereview.appspot.com/63153/diff/1/2#newcode274
Line 274: return
("__Pyx_GetBufferAndValidate(&%(pyxbufnd_struct)s.pyxbuf->bufinfo, "
See above comments here...

http://codereview.appspot.com/63153/diff/1/2#newcode682
Line 682: 
Not sure if I like the naming scheme with "pyxbuf"; I'm proposing something else
below, but feel free to improve further or ignore this.

http://codereview.appspot.com/63153/diff/1/2#newcode689
Line 689: Py_buffer bufinfo;
Py_buffer pybuffer;

http://codereview.appspot.com/63153/diff/1/2#newcode693
Line 693: __Pyx_Buffer *pyxbuf;
__Pyx_Buffer *buffer;

http://codereview.appspot.com/63153/diff/1/2#newcode696
Line 696: } __Pyx_Buffer_ND;
__Pyx_LocalBuf

or something like that -- something which makes it clear that this is redundant,
"stack-allocated" information, while __Pyx_Buffer is the real buffer.
__Pyx_Buffer_ND seems too close to __Pyx_Buffer when they are really on seperate
levels.

http://codereview.appspot.com/63153/diff/1/4
File Cython/Compiler/Naming.py (right):

http://codereview.appspot.com/63153/diff/1/4#newcode38
Line 38: pyxbufstruct_prefix  = pyrex_prefix + "pyxbuf_"
I think you can loose the extra pyx prefix here, otherwise it will look like

__pyx_pyxbuf_...

which is kind of redundant. (Perhaps try to find a more descriptive name for the
former, like "localbuf" or "buflocal")
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b