This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Use external editor in 'commands' command
- From: Tom Tromey <tromey at redhat dot com>
- To: Alfredo Ortega <ortegaalfredo at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 02 Feb 2009 15:17:51 -0700
- Subject: Re: Use external editor in 'commands' command
- References: <e598931c0901141343j79164cf6we2bc5307f41f41da@mail.gmail.com> <e598931c0901141347j4d159f16he23ad164760518ff@mail.gmail.com> <ueiz5wm3n.fsf@gnu.org> <e598931c0901141632m4f124d85l2452f7e2870cad91@mail.gmail.com> <uab9tw67k.fsf@gnu.org> <e598931c0901152338l2b1bde20r7e242167e038ea8f@mail.gmail.com> <u4ozzwrg2.fsf@gnu.org> <e598931c0901161408x5179b81fw113f4f9b0052e67b@mail.gmail.com> <uy6xa2sd8.fsf@gnu.org> <e598931c0901190345r449e9d0fpd0d6fc6f61f027d2@mail.gmail.com>
- Reply-to: tromey at redhat dot 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