Re: Use external editor in 'commands' command

Tom Tromey
2009-02-24T19:46:46Z

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

Alfredo> 2009-01-16 Alfredo Ortega <ortegaalfredo@gmail.com>
Alfredo> * breakpoint.c (commands_command,_initialize_breakpoint,
Alfredo> edit_command,check_executing_commands, commands_edit_command,
Alfredo> commands_dump_to_file):
Alfredo> Add the 'edit' keyword to the 'commands' command to allow the
Alfredo> use of an external editor to add or modify commands.
Alfredo> Added an utility function to dump breakpoint commands to a file.
Alfredo> Added an utility function to launch an external editor on breakpoint commands.
Alfredo> Joined common checks in commands_command and edit_command.

This is wordier than the norm. Also, a comment describing a change
should be next to the name of the function it describes. This seems
to list all the function names, then all the changes.

See the existing ChangeLog for a lot of examples. Also the GNU
standards have a section on writing a ChangeLog entry...

Alfredo> +commands_dump_to_file (char *filename, struct breakpoint *b)
Alfredo> +{
Alfredo> + struct cleanup *cleanups;
Alfredo> + struct ui_file *old, *outfile = gdb_fopen (filename, "w");

You have to check for a NULL return from gdb_fopen.

Alfredo> + do_cleanups (cleanups);
Alfredo> +
Alfredo> +}

Extra blank line in there.

Alfredo> +/* Launches the editor on the breakpoint command. */
Alfredo> +char *
Alfredo> +commands_edit_command (int bnum)

The header comment should describe the meaning of the return value and
the arguments. For a string return, like this, it should also
describe how the memory is managed.

Alfredo> + if (!vitmp)
Alfredo> + error (_("Can't create temporary file for editing."));

Two extra spaces of indentation on the second line, not four. This
occurs in a few spots.

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

The 'if' should go on a new line.
This will require some reindentation, as well.

Alfredo> + cmdline = xmalloc (strlen (editor) + strlen (vitmp) + 50);

Instead of 50, just use the correct number.
Or, use xstrprintf, which is simpler.

Alfredo> + if (sysret < 0)
Alfredo> + error (_("Can't execute external editor."));
Alfredo> + if (sysret > 0)
Alfredo> + error (_("External editor returned non-zero status."));

I think you need to use WEXITSTATUS and friends here.

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

Newline.

Alfredo> + {
Alfredo> + /* Redirect instream to the commands temporal file. */

I think this can be just "Read commands from the temporary file."

Alfredo> + instream = fopen (vitmp, "r");

Needs error checking.

Alfredo> + unlink (vitmp);

I suspect this should probably be done via a cleanup.

Alfredo> +/* List of subcommands for "edit". */
Alfredo> +static struct cmd_list_element *edit_command_list;

I don't think you need this...

Alfredo> + add_prefix_cmd ("edit", class_breakpoint, edit_command,_("\

... because I don't think "edit" needs to be a prefix command.
It can just be an ordinary command.

Alfredo> +extern char *external_editor( void );

Should be " (void)", not "( void )".

Alfredo> + add_setshow_filename_cmd ("external-editor", no_class, &external_editor_command, _("\

I don't remember... did we already discuss the name of the option?
Why "external-editor" and not the simpler "editor"?

Alfredo> +/* Returns the external editor */

Needs period and 2 spaces at the end.

Alfredo> + return editor;
Alfredo> +
Alfredo> +}

Extra blank line.

This is pretty close to ready. Thanks for persevering.

Tom