Re: PATCH: GDB/MI - implement -exec-abort

[Joel Brobecker <brobecker at adacore dot com>]

Dennis,

On Thu, Oct 23, 2014 at 08:25:21PM +0000, Dennis Brueni wrote:
> 
> The -execâabort command has been documented as the MI equivalent to the kill command since GDB 5.1, but it was never implemented.
> 
> This patch does that.
> 
> gdb/ChangeLog
> 2014-10-23  Dennis Brueni  <dbrueni@slickedit.com>
> 
> * mi/mi-cmds.c Add "exec-abortâ
> * mi/mi-cmds.h Add prototype for mi_cmd_exec_abort
> * mi/mi-main.c Add mi_cmd_exec_abort (identical to kill_command minus prompting)
> 
> gdb/doc/ChangeLog
> 2014-10-23  Dennis Brueni  <dbrueni@slickedit.com>
> 
> * doc/gdb.texinfo Revive -execâabort documentation, add example.

Sorry for the delay in reviewing your patch.

First things first, do you have an FSF copyright assignment on file?
I tried looking you up in the FSF records and couldn't find you.
Your patch is too big for us to be able to accept it. If you'd like
to be started on the process, please let me know, as it does take
a little bit of time to get through.

You didn't say how you tested your patch, and the headers of the diff
seem to indicate that the changes were made against GDB 7.8; instead,
we ask that patches be tested on top of our master branch, which is
the branch we use for development.

The formatting of your ChangeLog is not correct. Here is what they should
look like:

gdb/ChangeLog
2014-10-23  Dennis Brueni  <dbrueni@slickedit.com>

	* mi/mi-cmds.c (mi_cmds): Add "exec-abortâ command.
	* mi/mi-cmds.h (mi_cmd_exec_abort):  Add prototype.
	* mi/mi-main.c (mi_cmd_exec_abort): New function.

gdb/doc/ChangeLog
2014-10-23  Dennis Brueni  <dbrueni@slickedit.com>

	* doc/gdb.texinfo (GDB/MI Miscellaneous Commands): Revive
        -exec-abort documentation, add example.

More explicitly, I made the following adjustements:

  - the contents should be indented by a tab (warning: _not_ 8 spaces);
  - sentences should start with a capital letter and end with a period;
  - the parts you touch (eg: function name, @node name for the doco,
    should be listed in between parenthesis;
  - there should be a colon before you start explaning what you change;
  - There is max line length soft limit of 74 characters, and a hard
    limit of 80.

I am not sure how you formatted this message to send the patch,
but a recommended way to work is to commit your change with a revision
log that serves both as revision log as well as email to be sent
here for us to review. That way, we see exactly what is proposed,
revision log included, and it also helps archeology to have
the what, where, how and why parts attach to each patch. And, as
a side-effect, the patch will also use diff's "-p" option, which
provides the name of the function each hunk is in, making review
of your patch a bit easier.

I realize that's a lot of info to process for such a small patch,
and I am sorry to hit you with this, but consistency is important
to us.

> +/* Stop the execution of the target. */
> +
> +void
> +mi_cmd_exec_abort (char *command, char **argv, int argc)

First of all, thank you _very much_ for thinking of adding an
introductory comment for the function you're adding! Not everyone
does that, and it's also a rule that we want everyone to follow...

In terms of the function's implementation, however, I would rather
you re-used the code in kill_command rather than reimplement it
entirely like it.

So, can you please do the following instead:

  - Extract the code from kill_command out to a subprogram called
    for instance kill_current_inferior, with one argument used
    to tell the function whether the user should be queried
    or not.

    In the function's command, please make it clear that it uses
    INFERIOR_PTID to determine which inferior to kill.
    "kill_current_inferior" alludes to the "current_inferior" function
    which returns the value of another global. Intuitively, the should
    be the same, but I do not remember why we have two anymore, and
    we don't have to worry about that for your patch.

  - Make both kill_command and mi_cmd_exec_abort both call
    kill_current_inferior.

Thank you,
-- 
Joel