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

[Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>]

Hi Pedro,

Thanks for the review, and sorry for the delayed response.

On 25/07/2022 19:52, Pedro Alves wrote:
> 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.

I'm not sure why you mention it being a full frame_info object. root was 
never a usable frame_info_ptr, it was just used as a pointer to the 
frame_info_ptr list from the outside, and to make intrusive list 
operations easy as we don't have to check for an empty list. I did 
reword the comment to:

/* All frame_info_ptr objects are kept on an intrusive 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".  It is only used to simplify intrusive_list 
operations.  */

to hopefully explain things better.

>
>> +  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));
Ah, this was just inherited from github. I have fixed this for v4
>
-- 
Cheers,
Bruno