Re: Use external editor in 'commands' command

Tom Tromey
2009-02-02T22:18:06Z

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

Alfredo> 1) Following the suggestion of Tom Tromey, i made "commands"
Alfredo> a prefix command, and now it is "commands edit n".

Your patch does this using strncmp -- but gdb already has built-in
machinery for prefix commands. See add_prefix_cmd. So, the idea here
would be to change _initialize_breakpoint to use add_prefix_cmd,
instead of add_com, when creating the "commands" command. Then, you'd
have a separate function to implement "commands edit". Maybe this
means introducing a helper function to do some of the work, I don't
know.

Alfredo> This is a much better patch, but also is a much bigger one (I already
Alfredo> sent the FSF form that Tom suggested), so surely there are plenty of
Alfredo> errors. Corrections are welcomed.

A few nits inline.

Alfredo> +#define COMMANDS_EDCOMMAND "edit"

With the prefix change, you won't need this.

Alfredo> + /* Edit commands with external editor */

In the GNU style, comments are full sentences that start with a
capital letter and end with a period and two spaces. This one is
missing the period, but others are incorrect in other ways.

Alfredo> + /* discard the "edit" command */

E.g., this one...

Alfredo> + get_number (&p);
Alfredo> + bnum = get_number (&p);

Two calls to get_number seems suspect.

Alfredo> + /* vitmp = tempnam(NULL,".gdb"); this is more secure according to man mkstemp, but gcc complains... */

There's no need for this comment, IMO.

Alfredo> + if (!(vitmp = make_temp_file (NULL)))

GNU style prohibits assignments in conditionals.

Alfredo> + {
Alfredo> + error (_("Can't create temporary file for editing."));
Alfredo> + return;
Alfredo> + }

The "error" function never returns. It calls longjmp. So, this
return is not needed. This occurs in a few spots.

Alfredo> + l = b->commands;
Alfredo> + while (l)
Alfredo> + {
Alfredo> + fsize = 0;
Alfredo> + fsize += fwrite (l->line, 1, strlen (l->line), tmpstream);

I think you should probably use "print_command_lines" to print the
breakpoint commands to a file.

Alfredo> + sysret = system (cmdline);
Alfredo> + xfree (cmdline);
Alfredo> + if (sysret < 0)

I think this should also check "sysret" when it is >= 0, and fail if
the editor does not exit with status 0.

Thanks for working on this,
Tom