From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 94302 invoked by alias); 16 Sep 2018 02:11:12 -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 93917 invoked by uid 89); 16 Sep 2018 02:11:10 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= 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; Sun, 16 Sep 2018 02:11:08 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 8A28B1E16B; Sat, 15 Sep 2018 22:11:06 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=simark.ca; s=mail; t=1537063866; bh=41ALqoAm7xSKEOTAxIA0UyTVGC2QtyzH63yXmQmx7o8=; h=Subject:To:References:From:Date:In-Reply-To:From; b=dWUpis3tkjp0JDxlvENwq55j7Ks9A9KpD3L4PfTTaQPzx/PCGvf1TMeTH8XtNKPwe KwH7fgUX77UzJCYCXOrUUOkdVFlc56wFmekOYkbh50pUPqESyYM5OyTL7E+7y9/Dq3 9L3r0NwLH/fy+zG6cE5oKp3yDeNc2g8qzJuq4LdU= Subject: Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference To: Tom Tromey , gdb-patches@sourceware.org References: <20180913053007.11780-1-tom@tromey.com> <20180913053007.11780-4-tom@tromey.com> From: Simon Marchi Message-ID: <38dedd0d-1f03-eb7d-84a6-15cdc29e8ea2@simark.ca> Date: Sun, 16 Sep 2018 02:11:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: <20180913053007.11780-4-tom@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-09/txt/msg00516.txt.bz2 I have some comments about style, correctness-wise is looks good to me. On 2018-09-13 1:30 a.m., Tom Tromey wrote: > diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c > index 1cf37296973..bf8ac75a316 100644 > --- a/gdb/python/py-inferior.c > +++ b/gdb/python/py-inferior.c > @@ -306,7 +306,7 @@ find_inferior_object (int pid) > return NULL; > } > > -thread_object * > +gdbpy_ref<> > thread_to_thread_object (thread_info *thr) > { > gdbpy_ref inf_obj (inferior_to_inferior_object (thr->inf)); > @@ -317,7 +317,7 @@ thread_to_thread_object (thread_info *thr) > thread != NULL; > thread = thread->next) > if (thread->thread_obj->thread == thr) > - return thread->thread_obj; > + return gdbpy_ref<>::new_reference ((PyObject *) thread->thread_obj); > > return NULL; > } > @@ -817,7 +817,7 @@ infpy_is_valid (PyObject *self, PyObject *args) > PyObject * > infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw) > { > - PyObject *handle_obj, *result; > + PyObject *handle_obj; > inferior_object *inf_obj = (inferior_object *) self; > static const char *keywords[] = { "thread_handle", NULL }; > > @@ -826,8 +826,6 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw) > if (! gdb_PyArg_ParseTupleAndKeywords (args, kw, "O", keywords, &handle_obj)) > return NULL; > > - result = Py_None; > - > if (!gdbpy_is_value_object (handle_obj)) > { > PyErr_SetString (PyExc_TypeError, > @@ -835,29 +833,27 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw) > > return NULL; > } > - else > + > + gdbpy_ref<> result; > + TRY > + { > + struct thread_info *thread_info; > + struct value *val = value_object_to_value (handle_obj); > + > + thread_info = find_thread_by_handle (val, inf_obj->inferior); > + if (thread_info != NULL) > + result = thread_to_thread_object (thread_info); > + } > + CATCH (except, RETURN_MASK_ALL) > { > - TRY > - { > - struct thread_info *thread_info; > - struct value *val = value_object_to_value (handle_obj); > - > - thread_info = find_thread_by_handle (val, inf_obj->inferior); > - if (thread_info != NULL) > - { > - result = (PyObject *) thread_to_thread_object (thread_info); > - if (result != NULL) > - Py_INCREF (result); > - } > - } > - CATCH (except, RETURN_MASK_ALL) > - { > - GDB_PY_HANDLE_EXCEPTION (except); > - } > - END_CATCH > + GDB_PY_HANDLE_EXCEPTION (except); > } > + END_CATCH > > - return result; > + if (result == NULL) > + result = gdbpy_ref<>::new_reference (Py_None); I would suggest using Py_RETURN_NONE, which we already use at many places. Instead of setting that result variable, is there something preventing you from returning directly above, like this? if (thread_info != NULL) return thread_to_thread_object (thread_info).release (); The bottom line could just be unconditionally Py_RETURN_NONE. > + > + return result.release (); > } > > > diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c > index 36ae71b6358..4b2705a0af6 100644 > --- a/gdb/python/py-infthread.c > +++ b/gdb/python/py-infthread.c > @@ -284,15 +284,7 @@ PyObject * > gdbpy_selected_thread (PyObject *self, PyObject *args) > { > if (inferior_ptid != null_ptid) > - { > - PyObject *thread_obj > - = (PyObject *) thread_to_thread_object (inferior_thread ()); > - if (thread_obj != NULL) > - { > - Py_INCREF (thread_obj); > - return thread_obj; > - } > - } > + return thread_to_thread_object (inferior_thread ()).release (); I know it's logically not supposed to happen, but here it's possible for thread_to_thread_object to return NULL. Returning NULL to Python is the way to have it throw an exception, I believe, is that what you want to do here? The previous code would return None in that case. > > Py_RETURN_NONE; > } > diff --git a/gdb/python/py-stopevent.c b/gdb/python/py-stopevent.c > index f6bd207f3b8..cbdf7b7e7e0 100644 > --- a/gdb/python/py-stopevent.c > +++ b/gdb/python/py-stopevent.c > @@ -23,8 +23,8 @@ > gdbpy_ref<> > create_stop_event_object (PyTypeObject *py_type) > { > - return create_thread_event_object (py_type, > - py_get_event_thread (inferior_ptid)); > + gdbpy_ref<> thread = py_get_event_thread (inferior_ptid); > + return create_thread_event_object (py_type, thread.get ()); > } > > /* Callback observers when a stop event occurs. This function will create a > diff --git a/gdb/python/py-threadevent.c b/gdb/python/py-threadevent.c > index 4f822b4ae09..13af1c840ba 100644 > --- a/gdb/python/py-threadevent.c > +++ b/gdb/python/py-threadevent.c > @@ -22,19 +22,19 @@ > > /* See py-event.h. */ > > -PyObject * > +gdbpy_ref<> > py_get_event_thread (ptid_t ptid) > { > - PyObject *pythread = nullptr; > + gdbpy_ref<> pythread; > > if (non_stop) > { > thread_info *thread = find_thread_ptid (ptid); > if (thread != nullptr) > - pythread = (PyObject *) thread_to_thread_object (thread); > + pythread = thread_to_thread_object (thread); > } > else > - pythread = Py_None; > + pythread = gdbpy_ref<>::new_reference (Py_None); Again, I would suggest Py_RETURN_NONE. I would also propose moving the code below in the if, to show that it only really applies to the non-stop case (if I understand the code correctly). > > if (pythread == nullptr) > { Simon