This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 4/5] Make Python inferior-related internal functions return a gdbpy_inf_ref
- From: Pedro Alves <palves at redhat dot com>
- To: Simon Marchi <simon dot marchi at ericsson dot com>, gdb-patches at sourceware dot org
- Cc: Simon Marchi <simon dot marchi at polymtl dot ca>
- Date: Thu, 9 Feb 2017 12:30:31 +0000
- Subject: Re: [PATCH 4/5] Make Python inferior-related internal functions return a gdbpy_inf_ref
- Authentication-results: sourceware.org; auth=none
- References: <20170123224004.8893-1-simon.marchi@ericsson.com> <20170123224004.8893-5-simon.marchi@ericsson.com>
On 01/23/2017 10:40 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> The functions inferior_to_inferior_object and find_inferior_object
> return a new reference to an inferior_object. This means that the
> caller owns that reference and is responsible for decrementing it when
> it's done. To avoid the possibility of the caller forgetting to DECREF
> when it's done with the reference, make those functions return a
> gdbpy_inf_ref instead of a plain pointer.
I like this style of API. I've argued for it before too.
> If the caller doesn't need the reference after it has used it,
> gdbpy_inf_ref will take care of removing that reference. If the
> reference needs to outlive the gdbpy_inf_ref object (e.g. because we are
> return the value to Python, which will take ownership of the reference),
> the caller will have to release the pointer. At least it will be
> explicit and it won't be ambiguous.
>
> I added comments in inferior_to_inferior_object for the poor souls who
> will have to deal with this again in the future.
>
> A couple of things I am not sure about:
>
> * I am not sure whether the behaviour is right with the assignment
> operator in delete_thread_object, so if somebody could take a look at
> that in particular it would be appreciated:
>
> gdbpy_inf_ref inf_obj_ref = find_inferior_object (ptid_get_pid (tp->ptid));
>
> I suppose it's the operator= version which moves the reference that is
> invoked?
Since this is initialization, op= is not called. This either
calls the copy constructor, or find_inferior_object constructs the
object that it returns directly on top of &inf_obj_ref
(i.e., no copy at all) [RVO/NRVO].
http://stackoverflow.com/questions/2847787/constructor-or-assignment-operator
https://en.wikipedia.org/wiki/Return_value_optimization
> @@ -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.
>
> @@ -304,39 +303,34 @@ add_thread_object (struct thread_info *tp)
> static void
> delete_thread_object (struct thread_info *tp, int ignore)
> {
> - inferior_object *inf_obj;
> struct threadlist_entry **entry, *tmp;
>
> if (!gdb_python_initialized)
> return;
>
> gdbpy_enter enter_py (python_gdbarch, python_language);
> + gdbpy_inf_ref inf_obj_ref = find_inferior_object (ptid_get_pid (tp->ptid));
>
> - inf_obj
> - = (inferior_object *) find_inferior_object (ptid_get_pid (tp->ptid));
> - if (!inf_obj)
> + if (inf_obj_ref == NULL)
> return;
>
> /* 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?
> @@ -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.
> +
> + /* Release the reference, it will now be managed by Python. */
> + return (PyObject *) inf_obj_ref.release ();
> }
Thanks,
Pedro Alves