Re: Proposed patch for external debugger access

[John W. Eaton]

On 29-Oct-2007, John Swensen wrote:

| I simply copied this function from the help.cc file.  Since it was 
| declared "static" in the help.cc file, I could not access it from 
| debug.cc.  Is it possible to make this do_which() function available 
| from outside help.cc?  Then I wouldn't need to replicate it in debug.cc 
| and any changes to do_which() for the object branch would follow 
| directly in debug.cc.

If it is the same function, then I don't think there is any reason to
duplicate it.  We should remove the static qualifier from the function
in help.cc instead.

| >   // 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?
| >
| >   
| I'm not 100% sure about how to handle this one.  Currently in Matlab, if 
| you set a breakpoint using dbstop, it will place the breakpoint in 
| whichever file occurs first in the search path.  In fact, if you try to 
| add a breakpoint via their editor, it asks whether you want to add the 
| directory to the top of the search path or change the execution path to 
| the directory containing the file.  I'm not exactly sure how the 
| function overloading will work, so I guess I can't comment yet.  When a 
| user calls 's=dbstatus()' it will return the full path of the file in 
| which the breakpoint was placed in one of the structure fields.

OK.

| +intmap
| +bp_table::add_breakpoint (std::string fname, 
| +                       const intmap& line)
| +{
| +  intmap& linenc = const_cast<intmap&>(line);

Why are you casting away const here?  It seems the only use of linenc
is on the RHS of an assignment:

| +       int lineno = linenc[i];

so it seems that it should be OK to declare it const here.  Oh, intmap
is std::map<int,int> and there is no const operator [] method (because
the std::map operator [] always inserts an entry for a missing key) so
I think you need to use something like

  std::map<int, int>::const_iterator p = line.seek (i);

  if (p != line.end ())
    {
      int lineno = p->second;
      ...
    }
  else
    // Handle case of missing index...

BTW, I think the behavior would be unpredictible if you cast away
const and the operator [] method actually did insert something.

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

This should be "const std::string& fname".

| +      Cell namesCell (dim_vector (bp_list.size (), 1));
| +      Cell fileCell (dim_vector (bp_list.size (), 1));
| +      Cell lineCell (dim_vector (bp_list.size (), 1));

I usually find that it is better to avoid including the data type in
the variable name.

| +// A class to provide a standard interface to breakpoints, even if the 
| +// associated backend changes, we can make sure this higher level 
| +// interface stays the same.
| +class bp_table
| +{
| +public:
|
| [...]
|
| +};
| +
| +// Table of breakpoints
| +extern bp_table breakpoints;

Is there only one table of breakpoints?  If so, this should probably
be a singleton class.

jwe