Code review - Issue 195042: Review: bind refactorhttps://codereview.appspot.com/2010-01-26T23:31:47+00:00rietveld
Message from unknown
2010-01-26T19:28:17+00:00larrygritzurn:md5:674798960fe977c789136da8cf04624a
Message from larrygritz@gmail.com
2010-01-26T19:28:18+00:00larrygritzurn:md5:7574184d3da49735392e6207376e3374
Message from ckulla@gmail.com
2010-01-26T19:57:25+00:00ckullaurn:md5:11ccb558b3b0d167085aa223d4161258
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.
Message from larrygritz@gmail.com
2010-01-26T22:27:08+00:00larrygritzurn:md5:fdb8a63dd490f5f490316b9678501272
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 to floats even
> though they all have different types. Maybe a comment could explain why 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 to guarantee
> 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.
Message from unknown
2010-01-26T23:31:47+00:00larrygritzurn:md5:3c83e0ad6ac1ff99d0079ee294d99ee2