From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9182 invoked by alias); 18 Sep 2009 21:05:35 -0000 Received: (qmail 9173 invoked by uid 22791); 18 Sep 2009 21:05:34 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 18 Sep 2009 21:05:29 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n8IL5QoZ019854; Fri, 18 Sep 2009 17:05:27 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id n8IL5Qag027765; Fri, 18 Sep 2009 17:05:26 -0400 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id n8IL5PqT024895; Fri, 18 Sep 2009 17:05:25 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id 92AEF3782A5; Fri, 18 Sep 2009 15:05:24 -0600 (MDT) From: Tom Tromey To: Alfredo Ortega Cc: gdb-patches@sourceware.org Subject: Re: Use external editor in 'commands' command References: Reply-To: tromey@redhat.com Date: Fri, 18 Sep 2009 21:05:00 -0000 In-Reply-To: (Alfredo Ortega's message of "Fri, 28 Aug 2009 19:16:00 -0300") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-09/txt/msg00614.txt.bz2 >>>>> "Alfredo" == Alfredo Ortega writes: Alfredo> Sorry Tom for the very late answer. But I think I got all your Alfredo> recommendations implemented on the patch. Thanks. And, I'm also sorry for the delay in this response :-) Alfredo> Except that I couldn't find a way to do a cleanup erasing a file, Alfredo> without resorting to the ui_file functions. You can make a new handler function and directly call make_cleanup. Search for make_cleanup, there are plenty of examples of this. Alfredo> + if (outfile==NULL) Alfredo> + error (_("Can't create temporary file for editing.")); The if should be written: if (outfile == NULL). The GNU style puts spaces around operators. Also the second line is indented too far. Alfredo> +/* Launches the editor on the breakpoint command. Alfredo> + bnum is the breakpoint number. Write "BNUM" instead. See the coding styles for the reason. Alfredo> + ALL_BREAKPOINTS (b) Alfredo> + if (b->number == bnum) Alfredo> + { Alfredo> + if (&b->commands) This check is wrong. &b->commands will never be NULL. You meant just 'if (b->commands)'. Alfredo> + commands_dump_to_file (vitmp, b); Alfredo> + /* Edit the file. */ Alfredo> + cmdline = xstrprintf("%s \"%s\"",editor,vitmp); Indentation is weird. I guess you are mixing tabs and spaces. Exactly which to use is often discussed; but being inconsistent is worse than just picking one :-) Alfredo> + if (WEXITSTATUS(sysret) > 0) Add a space before the "(". This problem occurs in several places, please fix them all. (But this rule does not apply to "_("... fun :-) Alfredo> +static void Alfredo> +check_executing_commands () Use (void), not (). Alfredo> +/* Like commands_command but using an external editor. */ Alfredo> +static void Alfredo> +edit_command (char *args, int from_tty) Rather than duplicate a lot of commands_command, I think it would be preferable to refactor this: rename commands_command, add an argument indicating whether an editor should be used, and then have the new commands_command and edit_command call the internal function. Then you don't need check_executing_commands, either. Alfredo> + b->commands = l; Alfredo> + breakpoints_changed (); Alfredo> + observer_notify_breakpoint_modified (b->number); Also, update to CVS head. I think we're using breakpoint_set_commands now. Alfredo> + add_cmd ("edit", class_breakpoint, edit_command,_("\ Alfredo> +Edit the command using the external-editor variable, EDITOR environment\n\ Alfredo> +variable or /bin/ex, in that precedence."), I think this needs a bit more documentation. Maybe: Like `commands', but edit the commands using an editor. If there is an external editor (see `help set external-editor'), then that is used. Otherwise, if the EDITOR environment variable is set, then it is used. If all else fails, `/bin/ex' is used. Alfredo> +@table @code Alfredo> +@item set external-editor Alfredo> +@kindex set external-editor Alfredo> +This command controls the external text editor that internal gdb commands use. The external-editor variable has precedence over the @code{EDITOR} enviroment variable. This at least needs some line breaks. Tom