Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference

[Simon Marchi <simark at simark dot ca>]

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<inferior_object> 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