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

[Simon Marchi <simark@simark.ca>]

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

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

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

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

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`?

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

> +
> +  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;

Simon