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

Issue 199063: Review: extend inf/nan checking to derivatives, fix problem with arrays (Closed)

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

Description

Extend the inf/nan checking to also check (and print specific messages for) derivatives. Also, fixed a minor buglet that, for arrays, would only have checked the first element for inf/nan.

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -16 lines) Patch
src/liboslexec/exec.cpp View 4 chunks +30 lines, -15 lines 1 comment Download
src/liboslexec/oslexec_pvt.h View 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 5
larrygritz
13 years ago (2010-02-02 19:30:24 UTC) #1
ckulla
LGTM, except for the previously existing bug http://codereview.appspot.com/199063/diff/1/3 File src/liboslexec/exec.cpp (right): http://codereview.appspot.com/199063/diff/1/3#newcode346 src/liboslexec/exec.cpp:346: if (debugnan ...
13 years ago (2010-02-02 21:32:55 UTC) #2
lg_larrygritz.com
Good point. How about if I change the condition to: if (debugnan && (sym.symtype() == ...
13 years ago (2010-02-02 21:51:11 UTC) #3
ckulla
That works for me. Maybe throw in Const too? (just in case of some weird ...
13 years ago (2010-02-02 21:53:53 UTC) #4
lg_imageworks.com
13 years ago (2010-02-02 21:56:11 UTC) #5
Sure, I guess I could.  I figured that the constants from the oso file were
probably beyond reproach, but since it doesn't cost anything, will do.

	-- lg

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

> That works for me.
> 
> Maybe throw in Const too? (just in case of some weird oso parsing
> regression?)
> 
> http://codereview.appspot.com/199063/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