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

[Pedro Alves <pedro@palves.net>]

Hi Bruno,

On 2022-08-24 3:24 p.m., Bruno Larsen wrote:
> 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. 

I meant frame_info_ptr instead of frame_info, as that's what it was in the
original version:

 +  /* 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".  */
 +  static frame_info_ptr root;

> 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.

Not really.  The comment was there because the linked list implementation used one
element type as "root" (aka "head") object, a full frame_info_ptr object, same type as
all other elements, even if only its m_next/m_prev pointers were used.  This part:

  "To make the implementation a little simpler, we guarantee that there is always
   at least one object on the list this "root"."

... with the conversion to intrusive list, that comment no longer makes sense.  We don't
store a "root" object of element type any more.

In fact, I think you should rename the "root" to frame_list, or some such, like:

 - static intrusive_list<frame_info_ptr> root;
 + static intrusive_list<frame_info_ptr> frame_list;

... and don't talk about "root" any longer; however the head node is implemented, it's
an intrusive_list implementation detail.