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

[Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>]

Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:

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

That looks excellent.  Thanks.  I've included this change (to both _next
and _prev) in the v2 series I just posted.

Thanks,
Andrew