From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14201 invoked by alias); 24 Feb 2009 19:46:46 -0000 Received: (qmail 14190 invoked by uid 22791); 24 Feb 2009 19:46:44 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx2.redhat.com (HELO mx2.redhat.com) (66.187.237.31) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 24 Feb 2009 19:46:24 +0000 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n1OJk9Xi024591; Tue, 24 Feb 2009 14:46:09 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n1OJk9t4029167; Tue, 24 Feb 2009 14:46:09 -0500 Received: from opsy.redhat.com (vpn-12-148.rdu.redhat.com [10.11.12.148]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id n1OJk6ro010360; Tue, 24 Feb 2009 14:46:09 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id A50DF508295; Tue, 24 Feb 2009 12:46:04 -0700 (MST) To: Alfredo Ortega Cc: gdb-patches@sourceware.org Subject: Re: Use external editor in 'commands' command References: From: Tom Tromey Reply-To: tromey@redhat.com Date: Tue, 24 Feb 2009 20:20:00 -0000 In-Reply-To: (Alfredo Ortega's message of "Thu\, 19 Feb 2009 06\:28\:59 -0200") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (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-02/txt/msg00472.txt.bz2 >>>>> "Alfredo" == Alfredo Ortega writes: Alfredo> 2009-01-16 Alfredo Ortega Alfredo> * breakpoint.c (commands_command,_initialize_breakpoint, Alfredo> edit_command,check_executing_commands, commands_edit_command, Alfredo> commands_dump_to_file): Alfredo> Add the 'edit' keyword to the 'commands' command to allow the Alfredo> use of an external editor to add or modify commands. Alfredo> Added an utility function to dump breakpoint commands to a file. Alfredo> Added an utility function to launch an external editor on breakpoint commands. Alfredo> Joined common checks in commands_command and edit_command. This is wordier than the norm. Also, a comment describing a change should be next to the name of the function it describes. This seems to list all the function names, then all the changes. See the existing ChangeLog for a lot of examples. Also the GNU standards have a section on writing a ChangeLog entry... Alfredo> +commands_dump_to_file (char *filename, struct breakpoint *b) Alfredo> +{ Alfredo> + struct cleanup *cleanups; Alfredo> + struct ui_file *old, *outfile = gdb_fopen (filename, "w"); You have to check for a NULL return from gdb_fopen. Alfredo> + do_cleanups (cleanups); Alfredo> + Alfredo> +} Extra blank line in there. Alfredo> +/* Launches the editor on the breakpoint command. */ Alfredo> +char * Alfredo> +commands_edit_command (int bnum) The header comment should describe the meaning of the return value and the arguments. For a string return, like this, it should also describe how the memory is managed. Alfredo> + if (!vitmp) Alfredo> + error (_("Can't create temporary file for editing.")); Two extra spaces of indentation on the second line, not four. This occurs in a few spots. Alfredo> + ALL_BREAKPOINTS (b) if (b->number == bnum) The 'if' should go on a new line. This will require some reindentation, as well. Alfredo> + cmdline = xmalloc (strlen (editor) + strlen (vitmp) + 50); Instead of 50, just use the correct number. Or, use xstrprintf, which is simpler. Alfredo> + if (sysret < 0) Alfredo> + error (_("Can't execute external editor.")); Alfredo> + if (sysret > 0) Alfredo> + error (_("External editor returned non-zero status.")); I think you need to use WEXITSTATUS and friends here. Alfredo> + ALL_BREAKPOINTS (b) if (b->number == bnum) Newline. Alfredo> + { Alfredo> + /* Redirect instream to the commands temporal file. */ I think this can be just "Read commands from the temporary file." Alfredo> + instream = fopen (vitmp, "r"); Needs error checking. Alfredo> + unlink (vitmp); I suspect this should probably be done via a cleanup. Alfredo> +/* List of subcommands for "edit". */ Alfredo> +static struct cmd_list_element *edit_command_list; I don't think you need this... Alfredo> + add_prefix_cmd ("edit", class_breakpoint, edit_command,_("\ ... because I don't think "edit" needs to be a prefix command. It can just be an ordinary command. Alfredo> +extern char *external_editor( void ); Should be " (void)", not "( void )". Alfredo> + add_setshow_filename_cmd ("external-editor", no_class, &external_editor_command, _("\ I don't remember... did we already discuss the name of the option? Why "external-editor" and not the simpler "editor"? Alfredo> +/* Returns the external editor */ Needs period and 2 spaces at the end. Alfredo> + return editor; Alfredo> + Alfredo> +} Extra blank line. This is pretty close to ready. Thanks for persevering. Tom