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

Issue 195042: Review: bind refactor (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
14 years, 2 months ago by larrygritz
Modified:
14 years, 2 months ago
Reviewers:
ckulla
CC:
osl-dev_googlegroups.com, dev-osl_imageworks.com
Base URL:
http://openshadinglanguage.googlecode.com/svn/trunk/
Visibility:
Public.

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.

Patch Set 1 #

Total comments: 4

Patch Set 2 : Updated diff #

Unified diffs Side-by-side diffs Delta from patch set Stats (+140 lines, -108 lines) Patch
M src/liboslexec/context.cpp View 2 chunks +2 lines, -0 lines 0 comments Download
M src/liboslexec/exec.cpp View 1 4 chunks +111 lines, -106 lines 0 comments Download
M src/liboslexec/oslexec_pvt.h View 6 chunks +10 lines, -0 lines 0 comments Download
M src/liboslexec/shadingsys.cpp View 5 chunks +14 lines, -1 line 0 comments Download
M src/testshade/testshade.cpp View 3 chunks +3 lines, -1 line 0 comments Download

Messages

Total messages: 3
larrygritz
14 years, 2 months ago (2010-01-26 19:28:18 UTC) #1
ckulla
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> * ...
14 years, 2 months ago (2010-01-26 19:57:25 UTC) #2
larrygritz
14 years, 2 months ago (2010-01-26 22:27:08 UTC) #3
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.
Sign in to reply to this message.

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