Re: Proposed patch for external debugger access

[John W. Eaton]

On 24-Oct-2007, John Swensen wrote:

| I also added the filename associated with the function and breakpoints 
| in the struct returned by 's=dbstatus' (like Matlab).  I still don't 
| have the ability to pass the structure resulting from dbstatus into 
| dbstop, but I think I have ironed out the problems everywhere else.

OK, I have a few more comments.

+// Global variable containing the breakpoint information
+bp_table breakpoints;

There is no need to say that this is a global variable.  That's
clear.  I would simply write

  // Table of all breakpoints.

| +static void
| +parse_dbfunction_params (octave_value_list args, 
| +                      std::string& symbol_name, 
| +                      RowVector& lines)

You should never pass an object like octave_value_list by value.  It
should be const reference instead:

  static void
  parse_dbfunction_params (const octave_value_list& args, 
                           std::string& symbol_name, 
                           RowVector& lines)

Likewise for other arguments that are class objects.

| -  octave_value retval;
| +  octave_idx_type len = 0;
|    int nargin = args.length ();
|    int idx = 0;
| -  std::string symbol_name = "";
| +  int list_idx = 0;
| +  symbol_name = std::string ();
|  
| -  if (nargin != 1 && args(0).is_string())
| +  // If we are already in a debugging function
| +  if (octave_call_stack::caller_user_function () != NULL)
| +    {
| +      idx = 0;
| +    }
| +  else
|      {
| -      symbol_name = args(0).string_value ();
| +      symbol_name = args (0).string_value ();
|        idx = 1;
|      }

You should check for errors any time you extract a value from an
octave_value object.

| +     len += args (i).array_value ().length (); 

Maybe it would be better to just use

  len += args(i).numel ();

here?

| +  if (sym_rec && sym_rec->is_defined ())
| +    retval = sym_rec->which ();
| +  else
| +    retval = fcn_file_in_path (name);

This stuff is going to break when the object branch is merged.  I
don't think there's anything for you to do about that now, but I guess
it is worth mentioning it anyway.  If you have significant changes
that involve the symbol table, it might be best to defer them until
after 3.0 or to work on the object branch instead.

| +       if (static_cast<int> (retval.xelem (i)) != 0)

Instead of always using static_cast<int>, maybe it would be better to
store these in an Array<int> or some other object that has int
elements?

| +int bp_table::remove_breakpoint (std::string fname, RowVector line)

The arguments should be passed by const reference here.

| +RowVector bp_table::remove_all_breakpoints_in_file (std::string fname)

To match the style of the rest of Octave, please write the return type
on a separate line and put the function name at the beginning of the
next line.  That makes it easier to find the definition of a function
by doing something like "grep ^function_name *.cc".

Would it be more convenient internally to use std::list<int> or
std::set<int> instead of RowVector?  If you are concerned about being
able to convert it to something that you can stuff in an octave_value
object, it is easy enough to write a function to do the conversion to
a Matrix object when needed.  BTW, a conversion happens anyway since
there aren't actually any RowVector objects in the octave_value class
hierarchy (the octave_value (const RowVector&) constructor converts
the row vector to an NDArray object).

| +       int lineno = static_cast<int> (bkpts (i).matrix_value ()(0));

Why not "bkpts(i).int_value ()"?

| +      remove_all_breakpoints_in_file ((*it).first);

Please use it->first instead of (*it).first.

| --- /dev/null 2007-10-24 19:20:16.000000000 -0400
| +++ debug.h   2007-10-24 19:08:03.000000000 -0400
| @@ -0,0 +1,75 @@
| +/*
| +
| [...]
| +
| +*/
| +
| +#include <map>
| +#include "ov.h"

The contents of all header files should be protected from multiple
inclusion.  The convention is to use a macro named afer the file, so
this one would use "octave_debug_h".  Take a look at other files to
see how it is typically done in the Octave sources.

| +  // Default constructor
| +  bp_table (void) {};
| +
| +  // Destructor
| +  ~bp_table (void) {};

There's no need for these comments.  We know what these functions are.

| +  // Adds a breakpoint at the nearest executable line.
| +  RowVector add_breakpoint (std::string fname = "", 
| +                         RowVector lines = RowVector ());

Please write these comments as commands:

  // Add a breakpoint at the nearest executable line.

| +  // Returns a list of breakpoints in the system.  In the absence of 
| +  // the parameter, all breakpoints in the system are returned.
| +  std::map <std::string, RowVector> 
| +  get_breakpoint_list (octave_value_list fname_list);

If it returns a list, why is the return type a map?  Is it

  // Return all breakpoints.  Each element of the map is a vector
  // containing the breakpoints corresponding to a given function name.

?  What will happen when we have function overloading and there may be
more than one function with the same name?  Or is this name an
absolute file name?

| +  // The STL map containing a list of the pairwise 
| +  // <filename, octave_user_function*> for every function that 
| +  // contains at least one breakpoint.
| +  std::map< std::string, octave_user_function* > bp_map;

There is no need to say that this is an STL map as that is clear.  For
maps, I typically write something like

  // Map from function names to function objects for functions
  // containing at least one breakpoint.

| +extern bp_table breakpoints;

The comment for the definition should be duplicated here.

jwe