octave-maintainers
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Proposed patch for external debugger access


From: John Swensen
Subject: Re: Proposed patch for external debugger access
Date: Wed, 24 Oct 2007 07:24:30 -0400
User-agent: Thunderbird 2.0.0.6 (Macintosh/20070728)

John W. Eaton wrote:
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

Many of those were for the Doxygen commenting and for my personal preference (e.g. @param for Doxygen and //---... for visually dividing a file up). I will make the changes suggested and re-submit. Also, do I *need* to have my name in the copyright? Can I just revoke my rights as the copyright holder and make it public domain or bequeath the copyright to one of the main developers or the Octave project itself? This avoids the silly problem of having to contact all the copyright holder when some silly law requires you to do so.

After working with this a little more last night, I realized I have one more bug. My "sets" of breakpoints are being returned as lists, rather than row vectors. For both compatibility and simplicity reasons, they should definitely be row vectors. I also want to implement the functionality that a user can pass the structure(s) returned from dbstatus() into dbstop(). This is both compatible with Matlab and is quite useful for instances where you want to do the following:

s = dbstatus(); % Store all the breakpoints in the system (or a given file)
dbclear();        % Clear all breakpoints
<do something without worrying about a breakpoint interrupting execution>
dbstop (s);       % Restore all the breakpoints as they were before

John Swensen


reply via email to

[Prev in Thread] Current Thread [Next in Thread]