Re: [RFA] ppc: include register numbers in gdbarch_tdep structure.

[Elena Zannoni <ezannoni@cygnus.com>]

Kevin Buettner writes:
 > On Dec 10, 11:30am, Michael Snyder wrote:
 > 
 > > Elena Zannoni wrote:
 > > 
 > > > +++ ppc-bdm.c   2001/12/09 19:55:12
 > > > @@ -200,8 +200,8 @@ bdm_ppc_fetch_registers (int regno)
 > > >  /*      printf("Asking for register %d\n", first_regno); */
 > > > 
 > > >        /* if asking for an invalid register */
 > > > -      if ((first_regno == PPC_MQ_REGNUM) ||
 > > > -         ((first_regno >= FP0_REGNUM) && (first_regno <= FPLAST_REGNUM)))
 > > > +      if ((first_regno == gdbarch_tdep (current_gdbarch)->ppc_mq_regnum)
 > > > +         || ((first_regno >= FP0_REGNUM) && (first_regno <= FPLAST_REGNUM)))
 > > 
 > > [and many similar changes]
 > > 
 > > Not to be nit-picky, and I realize it's already been approved and
 > > committed, 
 > > but wouldn't this code look prettier if we simply provided something
 > > like:
 > > 
 > > #define PPC_MQ_REGNUM gdbarch_tdep (current_gdbarch)->ppc_mq_regnum
 > 
 > As with most things we do there are pros and cons.  On the pro side, I
 > agree that it looks prettier and is easier to read.  Also, having
 > these defines makes it easier to convert the code back to using actual
 > constants some day.  These are both excellent reasons to do as Michael
 > suggests.
 > 

Why would you want to convert the code back?  Anyway, I find it also
easier to debug the code, if you don't have a macro.

 > The drawback to using a macro like this is that it hides what's really
 > going on.  (But note that it is this very same quality that enhances
 > readability.)  In this case, the macro looks like a constant, leading
 > to the expectation that it is a constant and has the usual costs
 > associated with constants.  However, the runtime costs associated with
 > using this expression are significantly greater than using a constant.
 > 

But having a macro defined to be the same function call is not going
to speed up the evaluation.


 > That said, those of us accustomed to working on GDB are used to this
 > by now, aren't we?  E.g, consider:
 > 
 >     for (regno = 0; regno < NUM_REGS; regno++)
 >       ...
 > 
 > We all realize that a function is being called each time the test
 > ``regno < NUM_REGS'' is performed, don't we?

Sometimes, doing something because it was done in the past is not a
good metric. I think multiarch was developed leaving the macros
because it was a necessary thing to both minimize changes and keep
compatibility with non-multiarched targets. But here I would argue it
is a different situation.

 > 
 > If so, then I'm all in favor of Michael's suggestion.  (Actually, I'm
 > in favor of Michael's suggestion anyway.  But, I do think we need to
 > be more careful about how we write code that might potentially contain
 > a hidden function call.)
 > 

Sorry, I disagree.

 > Kevin

Elena