Use external editor in 'commands' command
Tom Tromey
tromey@redhat.com
Tue Feb 24 20:20:00 GMT 2009
>>>>> "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
More information about the Gdb-patches
mailing list