Re: [PATCH 06/24] gdbsupport: make intrusive_list's disposer accept a reference

Pedro Alves
2023-10-12T19:06:08Z

On 2023-10-10 21:40, Simon Marchi wrote:
> All the API of intrusive_list works with references to item, I'm not
> sure why the disposer used in intrusive_list::clear_and_dispose is
> different and receives a pointer. Change it to accept a reference
> instead. This helps simplify a bit a subsequent patch, and I don't see
> any downside to it.

The whole intrusive_list API was copied from Boost intrusive_list (not
the implementation), so that's where that came from.

I assume that the Disposer interface takes a pointer so that it has the same
interface as a unique_ptr Deleter, for instance. From a pure disposer/deleter angle,
it seems a bit odd to me to have a "delete" operator wrapper take a reference
instead of a pointer, leading to code like "delete &ref", FWIW.

If this doesn't result in a great tangible improvement, I'd prefer keeping
the interface in sync with Boost's.

Pedro Alves

>
> Change-Id: I380886cd4ddaf136fe532eb2744f9e69beb41283
> ---
> gdb/inferior.c | 10 +++++-----
> gdb/unittests/intrusive_list-selftests.c | 4 ++--
> gdbsupport/intrusive_list.h | 2 +-
> 3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index efe57cceae3e..4a55970ccba1 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -262,13 +262,13 @@ inferior::find_thread (ptid_t ptid)
> void
> inferior::clear_thread_list ()
> {
> - thread_list.clear_and_dispose ([=] (thread_info *thr)
> + thread_list.clear_and_dispose ([=] (thread_info &thr)
> {
> threads_debug_printf ("deleting thread %s",
> - thr->ptid.to_string ().c_str ());
> - set_thread_exited (thr, {}, true /* silent */);
> - if (thr->deletable ())
> - delete thr;
> + thr.ptid.to_string ().c_str ());
> + set_thread_exited (&thr, {}, true /* silent */);
> + if (thr.deletable ())
> + delete &thr;
> });
> ptid_thread_map.clear ();
> }
> diff --git a/gdb/unittests/intrusive_list-selftests.c b/gdb/unittests/intrusive_list-selftests.c
> index 1f85b3266830..d990c0878b35 100644
> --- a/gdb/unittests/intrusive_list-selftests.c
> +++ b/gdb/unittests/intrusive_list-selftests.c
> @@ -692,9 +692,9 @@ struct intrusive_list_test
> list.push_back (b);
> list.push_back (c);
>
> - auto disposer = [&] (const item_type *item)
> + auto disposer = [&] (const item_type &item)
> {
> - disposer_seen.insert (item);
> + disposer_seen.insert (&item);
> disposer_calls++;
> };
> list.clear_and_dispose (disposer);
> diff --git a/gdbsupport/intrusive_list.h b/gdbsupport/intrusive_list.h
> index 5e9243867d27..aea1922cabee 100644
> --- a/gdbsupport/intrusive_list.h
> +++ b/gdbsupport/intrusive_list.h
> @@ -513,7 +513,7 @@ class intrusive_list
> {
> while (!this->empty ())
> {
> - pointer p = &front ();
> + reference p = front ();
> pop_front ();
> disposer (p);
> }