From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1892 invoked by alias); 23 Nov 2014 07:18:40 -0000 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 Received: (qmail 1879 invoked by uid 89); 23 Nov 2014 07:18:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Sun, 23 Nov 2014 07:18:36 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id B1BEF1167BB; Sun, 23 Nov 2014 02:18:34 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id GD94aJeW7A3p; Sun, 23 Nov 2014 02:18:34 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 4AAB51166A5; Sun, 23 Nov 2014 02:18:34 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 6336C40F79; Sun, 23 Nov 2014 11:18:33 +0400 (RET) Date: Sun, 23 Nov 2014 07:18:00 -0000 From: Joel Brobecker To: Dennis Brueni Cc: "gdb-patches@sourceware.org" Subject: Re: PATCH: GDB/MI - implement -exec-abort Message-ID: <20141123071833.GA7136@adacore.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-11/txt/msg00554.txt.bz2 Dennis, On Thu, Oct 23, 2014 at 08:25:21PM +0000, Dennis Brueni wrote: > > The -exec–abort command has been documented as the MI equivalent to the kill command since GDB 5.1, but it was never implemented. > > This patch does that. > > gdb/ChangeLog > 2014-10-23 Dennis Brueni > > * mi/mi-cmds.c Add "exec-abort” > * mi/mi-cmds.h Add prototype for mi_cmd_exec_abort > * mi/mi-main.c Add mi_cmd_exec_abort (identical to kill_command minus prompting) > > gdb/doc/ChangeLog > 2014-10-23 Dennis Brueni > > * doc/gdb.texinfo Revive -exec–abort documentation, add example. Sorry for the delay in reviewing your patch. First things first, do you have an FSF copyright assignment on file? I tried looking you up in the FSF records and couldn't find you. Your patch is too big for us to be able to accept it. If you'd like to be started on the process, please let me know, as it does take a little bit of time to get through. You didn't say how you tested your patch, and the headers of the diff seem to indicate that the changes were made against GDB 7.8; instead, we ask that patches be tested on top of our master branch, which is the branch we use for development. The formatting of your ChangeLog is not correct. Here is what they should look like: gdb/ChangeLog 2014-10-23 Dennis Brueni * mi/mi-cmds.c (mi_cmds): Add "exec-abort” command. * mi/mi-cmds.h (mi_cmd_exec_abort): Add prototype. * mi/mi-main.c (mi_cmd_exec_abort): New function. gdb/doc/ChangeLog 2014-10-23 Dennis Brueni * doc/gdb.texinfo (GDB/MI Miscellaneous Commands): Revive -exec-abort documentation, add example. More explicitly, I made the following adjustements: - the contents should be indented by a tab (warning: _not_ 8 spaces); - sentences should start with a capital letter and end with a period; - the parts you touch (eg: function name, @node name for the doco, should be listed in between parenthesis; - there should be a colon before you start explaning what you change; - There is max line length soft limit of 74 characters, and a hard limit of 80. I am not sure how you formatted this message to send the patch, but a recommended way to work is to commit your change with a revision log that serves both as revision log as well as email to be sent here for us to review. That way, we see exactly what is proposed, revision log included, and it also helps archeology to have the what, where, how and why parts attach to each patch. And, as a side-effect, the patch will also use diff's "-p" option, which provides the name of the function each hunk is in, making review of your patch a bit easier. I realize that's a lot of info to process for such a small patch, and I am sorry to hit you with this, but consistency is important to us. > +/* Stop the execution of the target. */ > + > +void > +mi_cmd_exec_abort (char *command, char **argv, int argc) First of all, thank you _very much_ for thinking of adding an introductory comment for the function you're adding! Not everyone does that, and it's also a rule that we want everyone to follow... In terms of the function's implementation, however, I would rather you re-used the code in kill_command rather than reimplement it entirely like it. So, can you please do the following instead: - Extract the code from kill_command out to a subprogram called for instance kill_current_inferior, with one argument used to tell the function whether the user should be queried or not. In the function's command, please make it clear that it uses INFERIOR_PTID to determine which inferior to kill. "kill_current_inferior" alludes to the "current_inferior" function which returns the value of another global. Intuitively, the should be the same, but I do not remember why we have two anymore, and we don't have to worry about that for your patch. - Make both kill_command and mi_cmd_exec_abort both call kill_current_inferior. Thank you, -- Joel