Re: Use external editor in 'commands' command

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Alfredo" == Alfredo Ortega <ortegaalfredo@gmail.com> writes:

Alfredo> Sorry Tom for the very late answer. But I think I got all your
Alfredo> recommendations implemented on the patch.

Thanks.  And, I'm also sorry for the delay in this response :-)

Alfredo> Except that I couldn't find a way to do a cleanup erasing a file,
Alfredo> without resorting to the ui_file functions.

You can make a new handler function and directly call make_cleanup.
Search for make_cleanup, there are plenty of examples of this.

Alfredo> +  if (outfile==NULL)
Alfredo> +      error (_("Can't create temporary file for editing."));

The if should be written:  if (outfile == NULL).
The GNU style puts spaces around operators.

Also the second line is indented too far.

Alfredo> +/* Launches the editor on the breakpoint command.  
Alfredo> +   bnum is the breakpoint number.

Write "BNUM" instead.  See the coding styles for the reason.

Alfredo> +  ALL_BREAKPOINTS (b)
Alfredo> +    if (b->number == bnum)
Alfredo> +      {
Alfredo> +        if (&b->commands)

This check is wrong.  &b->commands will never be NULL.
You meant just 'if (b->commands)'.

Alfredo> +	  commands_dump_to_file (vitmp, b);
Alfredo> +        /* Edit the file.  */
Alfredo> +	cmdline = xstrprintf("%s \"%s\"",editor,vitmp);

Indentation is weird.  I guess you are mixing tabs and spaces.
Exactly which to use is often discussed; but being inconsistent is worse
than just picking one :-)

Alfredo> +        if (WEXITSTATUS(sysret) > 0)

Add a space before the "(".  This problem occurs in several places,
please fix them all.  (But this rule does not apply to "_("... fun :-)

Alfredo> +static void
Alfredo> +check_executing_commands ()

Use (void), not ().

Alfredo> +/* Like commands_command but using an external editor.  */
Alfredo> +static void
Alfredo> +edit_command (char *args, int from_tty)

Rather than duplicate a lot of commands_command, I think it would be
preferable to refactor this: rename commands_command, add an argument
indicating whether an editor should be used, and then have the new
commands_command and edit_command call the internal function.

Then you don't need check_executing_commands, either.

Alfredo> +        b->commands = l;
Alfredo> +        breakpoints_changed ();
Alfredo> +        observer_notify_breakpoint_modified (b->number);

Also, update to CVS head.  I think we're using breakpoint_set_commands now.

Alfredo> +  add_cmd ("edit", class_breakpoint, edit_command,_("\
Alfredo> +Edit the command using the external-editor variable, EDITOR environment\n\
Alfredo> +variable or /bin/ex, in that precedence."),

I think this needs a bit more documentation.  Maybe:

Like `commands', but edit the commands using an editor.
If there is an external editor (see `help set external-editor'), then
that is used.  Otherwise, if the EDITOR environment variable is set,
then it is used.  If all else fails, `/bin/ex' is used.

Alfredo> +@table @code
Alfredo> +@item set external-editor
Alfredo> +@kindex set external-editor
Alfredo> +This command controls the external text editor that internal gdb commands use. The external-editor variable has precedence over the @code{EDITOR} enviroment variable.

This at least needs some line breaks.

Tom