From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24032 invoked by alias); 10 Dec 2001 20:41:07 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 23978 invoked from network); 10 Dec 2001 20:41:06 -0000 Received: from unknown (HELO cygnus.com) (205.180.230.5) by sources.redhat.com with SMTP; 10 Dec 2001 20:41:06 -0000 Received: from rtl.cygnus.com (cse.cygnus.com [205.180.230.236]) by runyon.cygnus.com (8.8.7-cygnus/8.8.7) with ESMTP id MAA26653; Mon, 10 Dec 2001 12:41:03 -0800 (PST) Received: (from ezannoni@localhost) by rtl.cygnus.com (8.11.2/8.11.0) id fBAKl5x01727; Mon, 10 Dec 2001 15:47:05 -0500 X-Authentication-Warning: krustylu.cygnus.com: ezannoni set sender to ezannoni@cygnus.com using -f From: Elena Zannoni MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <15381.8009.857527.631618@krustylu.cygnus.com> Date: Mon, 10 Dec 2001 12:41:00 -0000 To: Kevin Buettner Cc: Michael Snyder , Elena Zannoni , gdb-patches@sources.redhat.com Subject: Re: [RFA] ppc: include register numbers in gdbarch_tdep structure. In-Reply-To: <1011210202327.ZM32506@ocotillo.lan> References: <15379.50110.429092.453327@krustylu.cygnus.com> <3C150D60.631D281C@cygnus.com> <1011210202327.ZM32506@ocotillo.lan> X-Mailer: VM 6.97 under Emacs 20.7.1 X-SW-Source: 2001-12/txt/msg00281.txt.bz2 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