Re: [PATCH 3/7] [gdbsupport] Factor out base_next_iterator

[Tom de Vries <tdevries@suse.de>]

On 4/30/26 6:24 PM, Simon Marchi wrote:
> On 4/23/26 2:35 AM, Tom de Vries wrote:
>> diff --git a/gdbsupport/next-iterator.h b/gdbsupport/next-iterator.h
>> index 0c90428d349..1dee941a252 100644
>> --- a/gdbsupport/next-iterator.h
>> +++ b/gdbsupport/next-iterator.h
>> @@ -21,27 +21,43 @@
>>   
>>   #include "gdbsupport/iterator-range.h"
>>   
>> -/* An iterator that uses the 'next' field of a type to iterate.  This
>> -   can be used with various GDB types that are stored as linked
>> -   lists.  */
>> +/* An iterator base class for iterating over a field of a type.  In order to
>> +   form a functioning iterator, classes inheriting this should define an
>> +   operator++, which determines the actual field that is iterated over.
>> +
>> +   Instead of factoring out a base class, we could use something like this:
>> +
>> +     template<typename T, auto F = &T::next>
>> +     struct next_iterator
>> +     {
>> +       ...
>> +       self_type &operator++ ()
>> +       {
>> +	 m_item = m_item->*F;
>> +	 return *this;
>> +       }
>> +       ...
>> +     }
>> +
>> +  but that has the drawback that it doesn't work with incomplete T.  */
> 

Hi Simon,

thanks for the review.

> I'm of the opinion that this information is not really relevant here, it
> belongs to the commit message.
> 

Ack, I've moved that to the commit message in a v1 ( 
https://sourceware.org/pipermail/gdb-patches/2026-May/227066.html ).

> I was initially skeptic that the version with the pointer-to-member
> didn't work, so I tried it for myself and indeed, I don't think it would
> work with the block and superblock_iterator relationship (at least, if
> we want to have a method of block returning a
> superblock_iterator/range).
> 

That was not what I was getting at.  The problem is that the gdb build 
breaks with the pointer-to-member approach because we exploit this 
incomplete type behavior in the sources.

> While next_iterator is "iterate using the raw field `next`",
> base_next_iterator is more generic just for "iterating over a field of a
> type".  It looks like you defined a pretty generic class where the
> derived class only needs to fill in operator++ (and perhaps a bit more
> boilerplate).  But operator++ doesn't have to just follow a field, it
> can have any logic in there, as function_block_iterator proves, which is
> nice.  Following the raw `next` field (or another raw field of any other
> name) is just one specific case.
> 
> Given that all the concrete iterate needs to provide is "given a
> reference to the current element, give me the next element", I think we
> could reduce the boilerplate needed at each site by having the concrete
> iterator provide a functor that does the increment (much like you
> defined an std::map by providing a "hash" functor, not by deriving from
> std::map).
> 

Tom Tromey suggested using CRTP, and doing so has I think brought it 
closer to what you suggest here.

> Rather than copy paste what I mean here, I pushed what I think it could
> look like to the users/simark/next-iterator branch.
> 
> https://sourceware.org/git/?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/next-iterator
> https://sourceware.org/cgit/binutils-gdb/log/?h=users/simark/next-iterator
> 
> But just to illustrate here is how the function block iterator and
> range are implemented:
> 
>    /* Iterator and range type to iterate over this block and its superblocks
>       within a function.  */
> 
>    struct function_block_incrementer
>    {
>      const block *operator() (const block &b) const noexcept
>      {
>        /* If the current item is the function-defining block, we're done.  */
>        if (b.function () != nullptr)
> 	return nullptr;
> 
>        return b.superblock ();
>      }
>    };
> 
>    using function_block_iterator
>      = base_next_iterator<const block, function_block_incrementer>;
>    using function_block_range = iterator_range<function_block_iterator>;
> 
> I kept the name `base_next_iterator`, we could perhaps find a better
> name, since it's not realted to `next_iterator` more than any other
> concrete instance.  Perhaps `base_iterator`, or `simple_iterator`?
> 

Yeah, agreed.  I kept it as is in v1 thought.

>> @@ -61,15 +77,34 @@ struct next_iterator
>>       return m_item != other.m_item;
>>     }
>>   
>> -  self_type &operator++ ()
>> +protected:
>> +
>> +  T *m_item;
>> +};
>> +
>> +/* An iterator that uses the 'next' field of a type to iterate.  This
>> +   can be used with various GDB types that are stored as linked
>> +   lists.  */
>> +
>> +template<typename T>
>> +struct next_iterator : base_next_iterator<T> {
>> +  typedef next_iterator self_type;
>> +  typedef T *value_type;
>> +  typedef T *&reference;
>> +  typedef T **pointer;
> 
> I think we should use `using` instead of typedef everywhere now.
> 

I've added a patch to the series doing this transformation in 
next-iterator.h and one other file.

>> +
>> +  explicit next_iterator (T *item)
>> +    : base_next_iterator<T> (item)
>>     {
>> -    m_item = m_item->next;
>> -    return *this;
>>     }
>>   
>> -private:
>> +  next_iterator () = default;
> 
> Just noting that instead of those two constructors, you can use:
> 
>    using base_next_iterator<T>::base_next_iterator;

Yeah, that's neat, thanks, I've used that in the v1.

Thanks,
- Tom