Re: [PATCH v3 2/5] Introduce frame_info_ptr smart pointer class

[Pedro Alves <pedro@palves.net>]

On 2022-07-25 6:06 p.m., Bruno Larsen via Gdb-patches wrote:

> +
> +private:
> +
> +  /* The underlying pointer.  */
> +  frame_info *m_ptr = nullptr;
> +
> +  /* All frame_info_ptr objects are kept on a circular doubly-linked
> +     list.  This keeps their construction and destruction costs
> +     reasonably small.  To make the implementation a little simpler,
> +     we guarantee that there is always at least one object on the list
> +     -- this "root".  */

This comment is stale -- this is no longer a full frame_info object.

> +  static intrusive_list<frame_info_ptr> root;


> diff --git a/gdbsupport/intrusive_list.h b/gdbsupport/intrusive_list.h
> index 6812266159a..48b2123582f 100644
> --- a/gdbsupport/intrusive_list.h
> +++ b/gdbsupport/intrusive_list.h
> @@ -391,13 +391,13 @@ class intrusive_list
>    void pop_front ()
>    {
>      gdb_assert (!this->empty ());
> -    erase_element (*m_front);
> +    erase (*m_front);
>    }
>  
>    void pop_back ()
>    {
>      gdb_assert (!this->empty ());
> -    erase_element (*m_back);
> +    erase (*m_back);
>    }
>  
>  private:
> @@ -451,7 +451,8 @@ class intrusive_list
>      m_back = &elem;
>    }
>  
> -  void erase_element (T &elem)
> +public:
> +  void erase (T &elem)
>    {
>      intrusive_list_node<T> *elem_node = as_node (&elem);
>  
> @@ -486,7 +487,6 @@ class intrusive_list
>      elem_node->prev = INTRUSIVE_LIST_UNLINKED_VALUE;
>    }
>  
> -public:
>    /* Remove the element pointed by I from the list.  The element
>       pointed by I is not destroyed.  */
>    iterator erase (const_iterator i)
> @@ -494,7 +494,7 @@ class intrusive_list
>      iterator ret = i;
>      ++ret;
>  
> -    erase_element (*i);
> +    erase (*i);
>  
>      return ret;
>    }
> 


API changes to utilities like this are better done in their own separate patch,
with a rationale, along with unit test changes (in this case, some new test in
gdb/unittests/intrusive_list-selftests.c).

However, intrusive_list's API is modeled on Boost's intrusive list, and there,
you see that there's no erase(T&) member:

 https://www.boost.org/doc/libs/1_67_0/doc/html/boost/intrusive/list.html

I'd rather not deviate unless there's a good reason.

AFAICT, you did the change for this:

 > +  ~frame_info_ptr ()
 > +  {
 > +    root.erase (*this);
 > +  }

which you should be able to tweak to use pre-existing API, like:

   root.erase (root.iterator_to (*this));