Re: Use external editor in 'commands' command

[Tom Tromey <tromey@redhat.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