Re: Use external editor in 'commands' command

Hi all,

Sorry Tom for the very late answer. But I think I got all your
recommendations implemented on the patch.
Except that I couldn't find a way to do a cleanup erasing a file,
without resorting to the ui_file functions.
But that wouldn't work because I need a *FILE for instream. Any hint?
Sending my latest patch anyway if someone want to take a look at it.
Regards,

Alfred

2009-08-28 Alfredo Ortega <ortegaalfredo <at> gmail.com>
 * breakpoint.c (commands_command,_initialize_breakpoint,
edit_command,check_executing_commands, commands_edit_command,
commands_dump_to_file):
Add the 'edit' keyword to the 'commands' command.
 * utils.c,defs.h (external_editor,initialize_utils):
 Added an utility function to return the external text editor of the system.
 Added new "set external-editor" and "show external-editor" commands.

2009-08-28 Alfredo Ortega <ortegaalfredo <at> gmail.com>
 * gdb.texinfo, refcard.tex (breakpoint commands, set
external-editor, show external-editor): Added
documentation of the edit option for editing commands.

2009/2/24 Tom Tromey <tromey <at> redhat.com>:
>>>>>> "Alfredo" == Alfredo Ortega <ortegaalfredo <at> gmail.com> writes:
>
> Alfredo> 2009-01-16 Alfredo Ortega <ortegaalfredo <at> 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
>