Proposed patch for external debugger access

[John W. Eaton]

On 23-Oct-2007, John Swensen wrote:

| I have attached a patch to address some of the issues with IDE's that 
| are going to want access to debugger information.

I'll try to evaluate this patch before 2.9.16/3.0, but it would help
if you could address the following comments and submit the patch
again.

| P.S. JWE, I still have the hardest time with your desired paren spacing 
| ;)  I do see your argument about it being more like the English 
| language, it is just that I am so used to doing it the other way that it 
| still seems awkward in terms of typing where I don't have that muscle 
| memory yet and I actually have to pause and force myself to do it.  
| However, I do think I caught all of them in this patch.

Please search for "( ", " )", and " ;" and fix those.

| +/**
| + * 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.
| + */

For consistency with most of the rest of Octave, please use C++ style
comments ("// comment").

| +class bp_table
| +{
| +public:
| +  /**
| +   * Default constructor
| +   */
| +  bp_table ();

To match the style of the rest of Octave, please use (void) instead of
just () for functions that don't take arguments.

| +  /**
| +   * Adds a breakpoint at the nearest executable line.
| +   *
| +   * @param fname The name of the m-file in which to set the breakpoint
| +   * @param line The line number on which to set the breakpoint.
| +   *
| +   * @return Returns the line number at which the breakpoint was
| +   * actually placed. This is only different than the input if the
| +   * input was not an executable line.
| +   */

What is the @param/@return for?  Are those doxygen markers?  I'm not
really convinced we need that for Octave, and it doesn't match the
style of the rest of the code.

| +  octave_value_list add_breakpoint (std::string fname = "", 
octave_value_list lines = octave_value_list());

Please try to avoid long lines like this.  Wrap them between
arguments if possible.

| +  /**
| +   * Returns a list of breakpoints in the system.  In the absence of the 
parameter, all
| +   * breakpoints are returned.  Using the parameter, a subset of the 
breakpoints can be
| +   * queried.
| +   *
| +   * @param fname_list A list of function names for which to check for 
breakpoints
| +   *
| +   * @return A map of the pairwise <file, linenumber>
| +   */
| +  std::map< std::string, octave_value_list > get_breakpoint_list ( 
octave_value_list fname_list );

Please write

  std::map <std::string, octave_value_list>
  get_breakpoint_list (octave_value_list fname_list);

| -Copyright (C) 2001, 2002, 2003, 2004, 2005, 2006, 2007 Ben Sapp
| +Copyright (C) 2001, 2002, 2003, 2004, 2005, 2006, 2007 Ben Sapp,
| +              2007 John Swensen

I think it would be best to just put

  Copyright (C) 2007 John Swensen

on a separate line.

| @@ -19,39 +20,14 @@
|  <http://www.gnu.org/licenses/>.
|  
|  */
| +#include "debug.h"

This should be in the list of other local includes, and definitely
after config.h, which should always be the first include file.

Oh, I see you moved all the includes to debug.h.  That's not really a
good thing to do.  A header like debug.h should only include the
minimum necessary for the header file itself, not everything for the
implementation of the class that is in the .cc file.

| 
+//-----------------------------------------------------------------------------------

I don't think other files in Octave have these kinds of markers, and
I'd prefer not to introduce them.  Why do you want them>

| +bp_table::bp_table ()
| +{
| + 
| +}

If a function is empty, it's definition can certainly go in the
declaration in the header file.

| +       cmds->delete_breakpoint ( static_cast<int> (line(i).matrix_value 
()(0)));

Always repeating line(i).matrix_value()(0) looks clumsy, so I think it
would be good to encapsulate it in another function.  Also, is it
possible for line(i) to be something other than a matrix object?  Is
it possible for it to be empty?  If so, then I think this could cause
trouble.

Sorry if some of these things seem silly, but it helps make the whole
of Octave easier to read if it follows a consistent style, so I will
change the style when I check in the changes, and if I'm doing that
it just slows things down.

jwe