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

[Pedro Alves <pedro@palves.net>]

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);
>        }