From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18910 invoked by alias); 19 Aug 2002 18:06:58 -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 18902 invoked from network); 19 Aug 2002 18:06:56 -0000 Received: from unknown (HELO mx1.redhat.com) (66.187.233.31) by sources.redhat.com with SMTP; 19 Aug 2002 18:06:56 -0000 Received: from int-mx2.corp.redhat.com (nat-pool-rdu.redhat.com [172.16.52.200] (may be forged)) by mx1.redhat.com (8.11.6/8.11.6) with ESMTP id g7JHqjl00713 for ; Mon, 19 Aug 2002 13:52:45 -0400 Received: from potter.sfbay.redhat.com (potter.sfbay.redhat.com [172.16.27.15]) by int-mx2.corp.redhat.com (8.11.6/8.11.6) with ESMTP id g7JI6lu23176; Mon, 19 Aug 2002 14:06:47 -0400 Received: from romulus.sfbay.redhat.com (remus.sfbay.redhat.com [172.16.27.252]) by potter.sfbay.redhat.com (8.11.6/8.11.6) with ESMTP id g7JI6ke07023; Mon, 19 Aug 2002 11:06:47 -0700 Received: (from kev@localhost) by romulus.sfbay.redhat.com (8.11.6/8.11.6) id g7JI6jF21014; Mon, 19 Aug 2002 11:06:45 -0700 Date: Mon, 19 Aug 2002 11:06:00 -0000 From: Kevin Buettner Message-Id: <1020819180645.ZM21013@localhost.localdomain> In-Reply-To: Elena Zannoni "[RFA] rs6000-tdep.c: pseudoregs infrastructure" (Aug 18, 1:40pm) References: <15711.56316.557645.578663@localhost.redhat.com> To: Elena Zannoni , gdb-patches@sources.redhat.com Subject: Re: [RFA] rs6000-tdep.c: pseudoregs infrastructure MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2002-08/txt/msg00553.txt.bz2 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