This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [RFA] rs6000-tdep.c: pseudoregs infrastructure
- From: Kevin Buettner <kevinb at redhat dot com>
- To: Elena Zannoni <ezannoni at redhat dot com>, gdb-patches at sources dot redhat dot com
- Date: Mon, 19 Aug 2002 11:06:45 -0700
- Subject: Re: [RFA] rs6000-tdep.c: pseudoregs infrastructure
- References: <15711.56316.557645.578663@localhost.redhat.com>
On Aug 18, 1:40pm, Elena Zannoni wrote:
> The only thing I am not sure is whether it wouldn't be better to add
> initialization values for the new fields added, instead of letting
> them deafult to 0. I think the code would be probably easier to
> maintain if I just added explicit zero's for those fields. yes? no?
Yes, I agree that maintenance will be easier if you add explicit
zeros to the non-pseudo-register initializer macros.
Now for some nits...
> @@ -2318,10 +2322,38 @@ struct variant
> /* Table of register names; registers[R] is the name of the register
> number R. */
> int nregs;
> + int npregs;
> + int num_tot_regs;
> const struct reg *regs;
> };
Could you add comments describing these new fields? (I'd appreciate
it too if you'd add a comment for nregs and move the comment
immediately above the nregs declaration to the correct place.)
> +int
> +num_registers(const struct reg *reg_list, int num_tot_regs)
> +{
> + int i;
> + int nregs = num_tot_regs;
> +
> + for (i = 0; i < num_tot_regs; i++)
> + if (reg_list[i].pseudo == 1)
> + nregs--;
> +
> + return nregs;
> +}
The ``pseudo'' field is intended to be a boolean, right? If so,
I think it'd be less surprising to see:
if (reg_list[i].pseudo)
instead of:
if (reg_list[i].pseudo == 1)
When I see the ``== 1'', I'm left wondering what other values this
field might take on.
The other thing that I found surprising about the above is that
you're counting down instead of up. We should get the same result
if we do the following instead, right? :
int i;
int nregs = 0;
for (i = 0; i < num_tot_regs; i++)
if (!reg_list[i].pseudo)
nregs++;
return nregs;
Structuring the code to count up instead of down will also cause it to
more closely resemble your definition of num_pseudo_registers(). In
fact, they'll be identical except for the ``reg_list[i].psuedo'' test.
....
The rest is okay. Feel free to commit this patch after adding the 0's
to the struct reg initializer macros and adding comments to
``struct variant''.
If you agree with my comments regarding num_registers(), then consider
changing it as I suggested. Otherwise, leave it alone. (I didn't
find it *that* hard to understand, but I spent slightly longer looking
at it than I might have otherwise.)
Thanks,
Kevin