On Feb 5, 2010, at 6:48 PM, <cku...@...> <cku...@...> wrote:

http://codereview.appspot.com/204046/diff/1/6#newcode386
src/liboslexec/exec.cpp:386: Runflag *orig =
context()->m_original_runflags;
Why is this save/restore needed?

I think it's not. I added it at one point that I was tracking down a bug and never removed it (figuring that it couldn't hurt, and might protect us from subtle bugs some day). I can remove it.

http://codereview.appspot.com/204046/diff/1/6#newcode534
src/liboslexec/exec.cpp:534: run (beginop, endop);
Why is m_conditional_level not set for this case?

Because that branch is for init ops. Um... In retrospect, I guess it can/should be set for both clauses. Let me try it and if nothing breaks I'll do it.

http://codereview.appspot.com/204046/diff/1/4#newcode110
src/liboslexec/opcontrol.cpp:110: exec->enter_conditional ();
Why is this needed? Can't it be handled inside push/pop runflags?

We push/pop runflags for other reasons. Such as in the main run() function. In that case, and possibly other future ones, pushing runflags doesn't necessarily imply that some points have been turned off, only that you want to save the existing runflags in case something later mucks with them. So I do enter/exit_conditional specifically in the places where we reduce to subsets of runflags, which is where it's needed.

http://codereview.appspot.com/204046/diff/1/2#newcode67
src/liboslexec/oslexec_pvt.h:67: #define NO_DEBUG_ADJUST_VARYING
I would prefer having:

// #define DEBUG_ADJUST_VARYING

Sure, why not.


--
Larry Gritz
l...@...