Re: tree walker evaluator

[John W. Eaton]

On  7-Jan-2009, John W. Eaton wrote:

| On  7-Jan-2009, David Bateman wrote:
| 
| | I notice this patch also comments out the MAYBE_DO_BREAKPOINT macros. 
| | Does that mean that breakpoints won't be correctly accepted? 
| | Unfortunately "make check" won't spot that type of regression with this 
| | change.
| 
| Yes, as it is currently written, that macro won't work outside the
| tree_*::eval member functions.  I'll see if I can make a debugging
| evaluator work before I check in the changes.  Thinking about this
| now, should it be possible to do debugging and profiling at the same
| time?

I checked in the following change:

  http://hg.savannah.gnu.org/hgweb/octave/rev/73c4516fae10

This is a fairly large modification to the interpreter (it touches 103
files) but I think it is better than what we had before and it passes
all the tests that were passing before the change, so I think it is
working correctly and is reasonably safe to commit now.

I think the debugger is now much improved (hey, I might even be able
to use it now :-).  Setting breakpoints appears to be much more
reliable and the implementation of dbstep is far simpler and seems to
work better than before.  Now you only step by statements, not
individual subexpressions, "dbstep in" and "dbstep out" should work
correctly, and "dbstep N" will not step past the end of the current
function.  When a breakpoint is reached, the source line is printed
before the "debug> " prompt is displayed.

The rvalue methods have been overhauled a bit.  Previously, we had

  octave_value_list rvalue (int nargout);

which was used to evaluate expressions that could produce any number
of output values, and

  octave_value rvalue (void);

which was used to evaluate expressions that were only supposed to
produce one output value.  But there was some confusion about how this
was used.

Now we have

  octave_value_list rvalue (int nargout);

which is the same as before, and

  octave_value rvalue1 (int nargout = 1);

which is used in places that evaluate expressions but will use only
one value, but in the given context may have nargout set to any
value (sometimes zero).  I'm not sure there were any cases where
Octave was producing incorrect results with the old functions, but
with the new versions the internals seem a little more
straightforward to me.

I was planning to have a separate tree_debugger class derived from the
tree_evaluator class and that would replace some of the functions in
the tree_evaluator class to do the debugging stuff before calling the
corresponding fucntion from the tree_evaluator class.  For example:

  void
  tree_debugger::visit_statement (tree_statement& stmt)
  {
    // check to see if breakpoint is set and debugging things...
    ...
    ...
    ...

    tree_evaluator::visit_statement (stmt);
  }

Although it is probably possible to make this work, it did not seem
worth the effort, and I wasn't sure I could correctly implement
switching between the normal and debugging evaluators at arbitrary
times.  So I decided to simply check a flag in the tree_evaluator
methods that need to do debugging things.  I don't think this adds
much overhead and it allows reliable switching between the normal and
debugging evaluators.

Comments?  Bug reports?

jwe