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

Issue 204077: check_nan bug fix and skip bind for struct params (Closed)

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

Description

Two small things I noticed while debugging something unrelated. 1) check_nan was off-by one 2) we allocate space for struct params even though they are never used directly I also added a bit more information to the check_nan output (including which point was nan).

Patch Set 1 #

Patch Set 2 : uploaded wrong diff #

Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -10 lines) Patch
src/liboslexec/exec.cpp View 1 8 chunks +21 lines, -9 lines 0 comments Download
src/liboslexec/oslexec_pvt.h View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 6
ckulla
14 years, 2 months ago (2010-02-08 19:58:25 UTC) #1
ckulla
uploaded wrong diff
14 years, 2 months ago (2010-02-08 20:00:29 UTC) #2
larrygritz
LGTM
14 years, 2 months ago (2010-02-08 20:07:27 UTC) #3
lg_imageworks.com
How much "space" were we allocating for the struct placeholders? Should be 0, no? Or ...
14 years, 2 months ago (2010-02-08 20:11:42 UTC) #4
ckulla
I would have to check again, it seems to me the size was > 0. ...
14 years, 2 months ago (2010-02-08 21:17:45 UTC) #5
lg_imageworks.com
14 years, 2 months ago (2010-02-08 22:19:31 UTC) #6
We should double-check that the placeholder struct symbol table entries don't
take any space in the heap.  They aren't intended to.

If you don't have time to do it, please just file a ticket and I'll take a look
later.

	-- lg


On Feb 8, 2010, at 1:17 PM, <ckulla@gmail.com> <ckulla@gmail.com> wrote:

> I would have to check again, it seems to me the size was > 0. In any
> case, it seemed dangerous to give a valid heap address to symbols that
> should never be used.
> 
> The nan fix is just that we were checking up to <= endpoint instead of <
> endpoint.
> 
> http://codereview.appspot.com/204077/show
> 

--
Larry Gritz
lg@imageworks.com




Sign in to reply to this message.

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