2009/2/19 Alfredo Ortega : > 2009/2/2 Tom Tromey : >>>>>>> "Alfredo" == Alfredo Ortega 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 > Sorry, I forgot the changelog in the last e-mail. I'm resending the patch. 2009-01-16 Alfredo Ortega * 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 to allow the use of an external editor to add or modify commands. Added an utility function to dump breakpoint commands to a file. Added an utility function to launch an external editor on breakpoint commands. Joined common checks in commands_command and edit_command. * utils.c,defs.h (external_editor,initialize_utils): Added an utility function to return the external text editor of the system. Added "set external-editor" and "show external-editor" commands to set/show the external editor variable 2009-01-16 Alfredo Ortega * gdb.texinfo, refcard.tex (breakpoint commands, set external-editor, show external-editor): Added documentation of the edit option, for editing commands with an external editor. Also, added a brief description of the "set external-editor" and "show external-editor" commands.