From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11195 invoked by alias); 16 Sep 2006 04:02:27 -0000 Received: (qmail 11181 invoked by uid 22791); 16 Sep 2006 04:02:26 -0000 X-Spam-Check-By: sourceware.org Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.31.1) with ESMTP; Sat, 16 Sep 2006 04:02:24 +0000 Received: from drow by nevyn.them.org with local (Exim 4.54) id 1GORNm-00021F-3j; Sat, 16 Sep 2006 00:02:22 -0400 Date: Sat, 16 Sep 2006 04:02:00 -0000 From: Daniel Jacobowitz To: Mike Frysinger Cc: gdb-patches@sourceware.org Subject: Re: [patch ping] change lookup order of $localvars to happen before symbol tables Message-ID: <20060916040222.GA7673@nevyn.them.org> Mail-Followup-To: Mike Frysinger , gdb-patches@sourceware.org References: <200608200903.07185.vapier@gentoo.org> <200608212132.30241.vapier@gentoo.org> <20060822013517.GA4935@nevyn.them.org> <200609140100.54614.vapier@gentoo.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200609140100.54614.vapier@gentoo.org> User-Agent: Mutt/1.5.13 (2006-08-11) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-09/txt/msg00081.txt.bz2 Sorry, I'm way behind on patches, and it seems others don't have much time for review either. On Thu, Sep 14, 2006 at 01:00:54AM -0400, Mike Frysinger wrote: > 2006-08-21 Mike Frysinger > > * value.h (lookup_only_internalvar): New prototype. > * value.c (lookup_only_internalvar): New function. > (lookup_internalvar): Use lookup_only_internalvar. > * parse.c (write_dollar_variable): Look up $ symbols in internal > table first rather than last. I think this approach is basically OK. Minor comments on the patch: > --- gdb/parse.c > +++ gdb/parse.c > @@ -486,6 +486,17 @@ write_dollar_variable (struct stoken str > if (i >= 0) > goto handle_register; > > + /* Any names starting with $ are probably debugger internal variables. */ > + > + if (str.ptr[0] == '$' && lookup_only_internalvar (copy_name (str) + 1)) > + { > +internal_lookup: > + write_exp_elt_opcode (OP_INTERNALVAR); > + write_exp_elt_intern (lookup_internalvar (copy_name (str) + 1)); > + write_exp_elt_opcode (OP_INTERNALVAR); > + return; > + } You don't need to check ptr[0]; this is write_dollar_variable, if it didn't start with a dollar sign we wouldn't be here :-) Scanning the list of internal variables is linear. Let's not be wasteful with it; please save the result the first time you do it. > @@ -508,12 +519,9 @@ write_dollar_variable (struct stoken str > return; > } > > - /* Any other names starting in $ are debugger internal variables. */ > - > - write_exp_elt_opcode (OP_INTERNALVAR); > - write_exp_elt_intern (lookup_internalvar (copy_name (str) + 1)); > - write_exp_elt_opcode (OP_INTERNALVAR); > - return; > + /* Any other names are assumed to be debugger internal variables. */ > + > + goto internal_lookup; Let's avoid goto, please, since it's only three lines. We're going to rescan the internal variable list here, but we don't much care. Would you mind resubmitting with those changes? -- Daniel Jacobowitz CodeSourcery