Date
1 - 3 of 3
Review: bind refactor (issue195042)
larry...@...
Reviewers: ,
Description: 1. Refactor "bind" to simplify the handling of globals. 2. Bug fix (exec.cpp:309) -- zero out local strings, the one place where uninitialized values in the heap can crash the renderer. 3. Infrastructure for eventual "rebind" optimization, including tracking the statistics on it and allowing an option to enable/disable. Please review this at http://codereview.appspot.com/195042/show Affected files: src/liboslexec/context.cpp src/liboslexec/exec.cpp src/liboslexec/oslexec_pvt.h src/liboslexec/shadingsys.cpp src/testshade/testshade.cpp |
|
cku...@...
LGTM - just a few small notes
http://codereview.appspot.com/195042/diff/1/5 File src/liboslexec/exec.cpp (right): http://codereview.appspot.com/195042/diff/1/5#newcode191 src/liboslexec/exec.cpp:191: VaryingRef<float> * valref = NULL, *dxref = NULL, *dyref = NULL; This is a big confusing - they all get turned into a VaryingRef to floats even though they all have different types. Maybe a comment could explain why this isn't important here. http://codereview.appspot.com/195042/diff/1/5#newcode310 src/liboslexec/exec.cpp:310: // Uninitialized strings in the heap can really screw Could we mark this as temporary? I really think it would be better to guarantee this on the compiler side. http://codereview.appspot.com/195042/show |
|
larry...@...
http://codereview.appspot.com/195042/diff/1/5
File src/liboslexec/exec.cpp (right): http://codereview.appspot.com/195042/diff/1/5#newcode191 src/liboslexec/exec.cpp:191: VaryingRef<float> * valref = NULL, *dxref = NULL, *dyref = NULL; On 2010/01/26 19:57:25, ckulla wrote: This is a big confusing - they all get turned into a VaryingRef tofloats even though they all have different types. Maybe a comment could explainwhy this isn't important here.The idea is that the VaryingRef is really just a pointer underneath, so I'm combining the triple and float cases rather than have two sets of variables. I'll put in a comment and re-submit. http://codereview.appspot.com/195042/diff/1/5#newcode310 src/liboslexec/exec.cpp:310: // Uninitialized strings in the heap can really screw On 2010/01/26 19:57:25, ckulla wrote: Could we mark this as temporary? I really think it would be better toguarantee this on the compiler side.I'm not sure I agree. Why clutter up the instruction stream with initializations? It's only a runtime implementation issue that the way we store strings can lead to bad things for uninitialized memory in the string variable. If this weren't the case (as it may not be for a different "back end"), we wouldn't bother with this, necessarily. But I'll put in a comment here that at least points out the possibility that it could be solved in the compiler. http://codereview.appspot.com/195042/show |
|