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