From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 110718 invoked by alias); 9 Feb 2017 16:39:26 -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 110703 invoked by uid 89); 9 Feb 2017 16:39:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy=H*u:1.2.3, H*UA:1.2.3, HX-PHP-Originating-Script:rcube.php, ref_ptr 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; Thu, 09 Feb 2017 16:39:23 +0000 Received: by simark.ca (Postfix, from userid 33) id 0F8D31EA09; Thu, 9 Feb 2017 11:39:22 -0500 (EST) To: Pedro Alves Subject: Re: [PATCH 4/5] Make Python inferior-related internal functions return a gdbpy_inf_ref 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: Thu, 09 Feb 2017 16:39:00 -0000 From: Simon Marchi Cc: Simon Marchi , gdb-patches@sourceware.org, Tom Tromey In-Reply-To: <0bf1c645-7efb-4348-feee-5c848f71fee8@redhat.com> References: <20170123224004.8893-1-simon.marchi@ericsson.com> <20170123224004.8893-5-simon.marchi@ericsson.com> <0bf1c645-7efb-4348-feee-5c848f71fee8@redhat.com> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.3 X-IsSubscribed: yes X-SW-Source: 2017-02/txt/msg00230.txt.bz2 Thanks for the comments. I'll update my branch, but I'll wait until Tom's series is pushed and see what's still relevant in mine. On 2017-02-09 07:30, Pedro Alves wrote: >> @@ -207,39 +207,38 @@ python_new_objfile (struct objfile *objfile) >> representing INFERIOR. If the object has already been created, >> return it and increment the reference count, otherwise, create >> it. >> Return NULL on failure. */ >> -inferior_object * >> +gdbpy_inf_ref >> inferior_to_inferior_object (struct inferior *inferior) >> { > ... >> - if (!inf_obj) >> - return NULL; >> + if (inf_obj == NULL) >> + return gdbpy_inf_ref (); > > You shouldn't need changes like this one. gdbpy_ref has an > implicit ctor that takes nullptr_t exactly to allow implicit > construction from null. Ok. This required adding the corresponding constructor in gdbpy_ref_base: gdbpy_ref_base (const std::nullptr_t) : gdb::ref_ptr> (nullptr) { } >> /* Find thread entry in its inferior's thread_list. */ >> - for (entry = &inf_obj->threads; *entry != NULL; entry = >> - &(*entry)->next) >> + for (entry = &inf_obj_ref.get ()->threads; > > Hmm, changes like these are odd. gdbpy_ref has an operator-> > implementation, so inf_obj->threads should do the right thing? Hmm you're right, not sure why I added those. >> @@ -815,7 +809,10 @@ py_free_inferior (struct inferior *inf, void >> *datum) >> PyObject * >> gdbpy_selected_inferior (PyObject *self, PyObject *args) >> { >> - return (PyObject *) inferior_to_inferior_object (current_inferior >> ()); >> + gdbpy_inf_ref inf_obj_ref (inferior_to_inferior_object >> (current_inferior ())); > > If the function returns gdbpy_inf_ref already, I much prefer > using = initialization over (), like: > > gdbpy_inf_ref inf_obj_ref > = inferior_to_inferior_object (current_inferior ()); > > The reason is that this makes it more obvious what is going on. > The ctor taking a PyObject* is explicit so inferior_to_inferior_object > must be returning a gdbpy_inf_ref. > > With: > > gdbpy_inf_ref inf_obj_ref (inferior_to_inferior_object > (current_inferior ())); > > one has to wonder what constructor is being called, and whether there's > some kind of explicit conversion going on. > > So the = version is more to the point and thus makes it > for a clearer read because there's less to reason about. Right, it's more obvious. Thanks, Simon