Re: [PATCH 11/16] gdb: remove reggroup_next and reggroup_prev

[Lancelot SIX <lsix@lancelotsix.com>]

> diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
> index b968947fa1c..65ec8241370 100644
> --- a/gdb/tui/tui-regs.c
> +++ b/gdb/tui/tui-regs.c
> @@ -523,10 +523,21 @@ tui_data_item_window::rerender (WINDOW *handle, int field_width)
>  static const reggroup *
>  tui_reg_next (const reggroup *current_group, struct gdbarch *gdbarch)
>  {
> -  const reggroup *group = reggroup_next (gdbarch, current_group);
> -  if (group == NULL)
> -    group = reggroup_next (gdbarch, NULL);
> -  return group;
> +  const std::vector<const reggroup *> &groups = gdbarch_reggroups (gdbarch);
> +
> +  if (current_group != nullptr)
> +    for (int i = 0; i < groups.size (); ++i)
> +      {
> +	if (groups[i] == current_group)
> +	  {
> +	    if (i + 1 < groups.size ())
> +	      return groups[i + 1];
> +	    else
> +	      return groups.front ();
> +	  }
> +      }
> +
> +  return groups.front ();
>  }
>  
>  /* Helper for "tui reg prev", wraps a call to REGGROUP_PREV, but adds wrap
> @@ -538,10 +549,27 @@ tui_reg_next (const reggroup *current_group, struct gdbarch *gdbarch)
>  static const reggroup *
>  tui_reg_prev (const reggroup *current_group, struct gdbarch *gdbarch)
>  {
> -  const reggroup *group = reggroup_prev (gdbarch, current_group);
> -  if (group == NULL)
> -    group = reggroup_prev (gdbarch, NULL);
> -  return group;
> +  const std::vector<const reggroup *> &groups = gdbarch_reggroups (gdbarch);
> +
> +  if (current_group != nullptr)
> +    {
> +      /* Iterate backwards.  */
> +      for (auto iter = groups.rbegin ();
> +	   iter != groups.rend ();
> +	   ++iter)
> +	{
> +	  if (*iter == current_group)
> +	    {
> +	      ++iter;
> +	      if (iter == groups.rend ())
> +		return groups.back ();
> +	      else
> +		return *iter;
> +	    }
> +	}
> +    }
> +
> +  return groups.back ();
>  }
>  

Hi,

It looks strange to have tui_reg_next implemented using iteration over
indexes while tui_reg_prev is implemented using iterators.  It looks to
me that both cases are similar enough to use a similar pattern.

Also, the for loop can probably be replaced by a call to std::find which
the overall thing more concise:

    static const reggroup *
    tui_reg_prev (const reggroup *current_group, struct gdbarch *gdbarch)
    {
      const std::vector<const reggroup *> &groups = gdbarch_reggroups (gdbarch);
      auto it = std::find (groups.rbegin (), groups.rend (), current_group);
      it++;
      if (it >= groups.rend ())
        return groups.back ();
      return *it;
    }

and something similar for tui_reg_next, changing rbegin/rend with
begin/end and .back with .front.

WDYT?
Best,
Lancelot.