This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Use external editor in 'commands' command
- From: Tom Tromey <tromey at redhat dot com>
- To: Alfredo Ortega <ortegaalfredo at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 24 Feb 2009 12:46:04 -0700
- Subject: Re: Use external editor in 'commands' command
- References: <e598931c0901141343j79164cf6we2bc5307f41f41da@mail.gmail.com> <e598931c0901141632m4f124d85l2452f7e2870cad91@mail.gmail.com> <uab9tw67k.fsf@gnu.org> <e598931c0901152338l2b1bde20r7e242167e038ea8f@mail.gmail.com> <u4ozzwrg2.fsf@gnu.org> <e598931c0901161408x5179b81fw113f4f9b0052e67b@mail.gmail.com> <uy6xa2sd8.fsf@gnu.org> <e598931c0901190345r449e9d0fpd0d6fc6f61f027d2@mail.gmail.com> <m38wooo54g.fsf@fleche.redhat.com> <e598931c0902190020h73744de0nfe54902bb350174e@mail.gmail.com> <e598931c0902190028x47e79037x1746e7c3f0c70726@mail.gmail.com>
- Reply-to: tromey at redhat dot com
>>>>> "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