From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 97731 invoked by alias); 10 Apr 2017 02:36:02 -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 97718 invoked by uid 89); 10 Apr 2017 02:36:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy=act X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 10 Apr 2017 02:35:58 +0000 Received: by simark.ca (Postfix, from userid 33) id 8B5D11E7F5; Sun, 9 Apr 2017 22:35:58 -0400 (EDT) To: Tom Tromey Subject: Re: [RFA 02/14] Introduce command_line_up X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 10 Apr 2017 02:36:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org In-Reply-To: <20170408201208.2672-3-tom@tromey.com> References: <20170408201208.2672-1-tom@tromey.com> <20170408201208.2672-3-tom@tromey.com> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.4 X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00219.txt.bz2 On 2017-04-08 16:11, Tom Tromey wrote: > This introduces command_line_up, a unique_ptr for command_line > objects, and changes many places to use it. This removes a number of > cleanups. > > Command lines are funny in that sometimes they are reference counted. > Once there is more C++-ification of some of the users, perhaps all of > these can be changed to use shared_ptr instead. Looks good to me in general, just a few comments. > diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c > index 33b657d..ded2d2b 100644 > --- a/gdb/cli/cli-script.c > +++ b/gdb/cli/cli-script.c > @@ -165,27 +165,22 @@ build_command_line (enum command_control_type > type, const char *args) > /* Build and return a new command structure for the control commands > such as "if" and "while". */ > > -struct command_line * > +command_line_up > get_command_line (enum command_control_type type, const char *arg) > { > - struct command_line *cmd; > - struct cleanup *old_chain = NULL; > + command_line_up cmd; > > /* Allocate and build a new command line structure. */ > - cmd = build_command_line (type, arg); > - > - old_chain = make_cleanup_free_command_lines (&cmd); > + cmd.reset (build_command_line (type, arg)); Can you do command_line_up cmd (build_command_line (type, arg)); ? > @@ -1256,17 +1243,17 @@ read_command_lines (char *prompt_arg, int > from_tty, int parse_commands, > /* Act the same way as read_command_lines, except that each new line > is > obtained using READ_NEXT_LINE_FUNC. */ > > -struct command_line * > +command_line_up > read_command_lines_1 (char * (*read_next_line_func) (void), int > parse_commands, > void (*validator)(char *, void *), void *closure) > { > - struct command_line *head, *tail, *next; > - struct cleanup *old_chain = make_cleanup (null_cleanup, NULL); > + struct command_line *tail, *next; > + command_line_up head; > enum command_control_type ret; > enum misc_command_type val; > > control_level = 0; > - head = tail = NULL; > + tail = NULL; > > while (1) > { > @@ -1307,18 +1294,17 @@ read_command_lines_1 (char * > (*read_next_line_func) (void), int parse_commands, > } > else > { > - head = next; > - make_cleanup_free_command_lines (&head); > + /* Make sure not to free HEAD. */ > + head.release (); > + head.reset (next); I don't understand that bit, why the comment and the release? From what I understand, this else clause is executed only for the first item, when head is still NULL. The goal being to keep a reference to the head of the list. All the other iterations will go in the true clause, which appends the new item at the end of the list. > } > tail = next; > } > > dont_repeat (); > > - if (ret != invalid_control) > - discard_cleanups (old_chain); > - else > - do_cleanups (old_chain); > + if (ret == invalid_control) > + head.reset (NULL); I wonder if a simple "return NULL" would do the trick here. Either way is fine with me. Thanks, Simon