Re: Use external editor in 'commands' command

Tom Tromey
2009-09-18T21:05:35Z

>>>>> "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