Re: Use external editor in 'commands' command

[Tom Tromey <tromey@redhat.com>]

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

Alfredo> 1) Following the suggestion of Tom Tromey, i made "commands"
Alfredo> a prefix command, and now it is "commands edit n".

Your patch does this using strncmp -- but gdb already has built-in
machinery for prefix commands.  See add_prefix_cmd.  So, the idea here
would be to change _initialize_breakpoint to use add_prefix_cmd,
instead of add_com, when creating the "commands" command.  Then, you'd
have a separate function to implement "commands edit".  Maybe this
means introducing a helper function to do some of the work, I don't
know.

Alfredo> This is a much better patch, but also is a much bigger one (I already
Alfredo> sent the FSF form that Tom suggested), so surely there are plenty of
Alfredo> errors. Corrections are welcomed.

A few nits inline.

Alfredo> +#define COMMANDS_EDCOMMAND "edit"

With the prefix change, you won't need this.

Alfredo> +  /* Edit commands with external editor */

In the GNU style, comments are full sentences that start with a
capital letter and end with a period and two spaces.  This one is
missing the period, but others are incorrect in other ways.

Alfredo> +      /* discard the "edit" command */

E.g., this one...

Alfredo> +      get_number (&p);
Alfredo> +      bnum = get_number (&p);

Two calls to get_number seems suspect.

Alfredo> +      /* vitmp = tempnam(NULL,".gdb"); this is more secure according to man mkstemp, but gcc complains... */

There's no need for this comment, IMO.

Alfredo> +      if (!(vitmp = make_temp_file (NULL)))

GNU style prohibits assignments in conditionals.

Alfredo> +	{
Alfredo> +	  error (_("Can't create temporary file for editing."));
Alfredo> +	  return;
Alfredo> +	}

The "error" function never returns.  It calls longjmp.  So, this
return is not needed.  This occurs in a few spots.

Alfredo> +	      l = b->commands;
Alfredo> +	      while (l)
Alfredo> +		{
Alfredo> +		  fsize = 0;
Alfredo> +		  fsize += fwrite (l->line, 1, strlen (l->line), tmpstream);

I think you should probably use "print_command_lines" to print the
breakpoint commands to a file.

Alfredo> +	  sysret = system (cmdline);
Alfredo> +	  xfree (cmdline);
Alfredo> +	  if (sysret < 0)

I think this should also check "sysret" when it is >= 0, and fail if
the editor does not exit with status 0.

Thanks for working on this,
Tom