Re: [RFA] rs6000-tdep.c: pseudoregs infrastructure

[Kevin Buettner <kevinb@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