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

Andrew Burgess via Gdb-patches
2022-04-06T12:14:16Z

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