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

Lancelot SIX via Gdb-patches
2022-04-05T23:12:24Z

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