[06/24] gdbsupport: make intrusive_list's disposer accept a reference
Commit Message
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.
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(-)
Comments
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);
> }
On 10/12/23 15:05, Pedro Alves wrote:
> 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.
I forgot about that, that's a good reason, I'll drop this patch.
Thanks,
Simon
@@ -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 ();
}
@@ -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);
@@ -513,7 +513,7 @@ class intrusive_list
{
while (!this->empty ())
{
- pointer p = &front ();
+ reference p = front ();
pop_front ();
disposer (p);
}