2009/2/2 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
>

Sorry for the late answer.
I refactored the whole patch, is getting better...add_prefix_cmd()
made me sweat! :) and print_command_lines() is a little tricky to use,
so maybe i didn't get it right this time.
Also I send the FSF paperwork and tried to do all your corrections in
the following patch.
Thanks to you for teaching and the patience!

Regards,

Alfred