From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Received: (qmail 408 invoked from network); 11 Nov 2002 22:33:05 -0000 Received: from unknown (HELO mx1.redhat.com) (66.187.233.31) by sources.redhat.com with SMTP; 11 Nov 2002 22:33:05 -0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.11.6/8.11.6) with ESMTP id gABMA2w25146 for ; Mon, 11 Nov 2002 17:10:02 -0500 Received: from pobox.corp.redhat.com (pobox.corp.redhat.com [172.16.52.156]) by int-mx1.corp.redhat.com (8.11.6/8.11.6) with ESMTP id gABMX4D12007; Mon, 11 Nov 2002 17:33:04 -0500 Received: from localhost.redhat.com (romulus-int.sfbay.redhat.com [172.16.27.46]) by pobox.corp.redhat.com (8.11.6/8.11.6) with ESMTP id gABMX1q32625; Mon, 11 Nov 2002 17:33:02 -0500 Received: by localhost.redhat.com (Postfix, from userid 469) id 950EEFF79; Mon, 11 Nov 2002 17:28:46 -0500 (EST) From: Elena Zannoni MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <15824.12061.128364.623571@localhost.redhat.com> Date: Mon, 11 Nov 2002 17:15:00 -0000 To: David Carlton Cc: gdb-patches@sources.redhat.com, Elena Zannoni , Fernando Nasser , Jim Blandy Subject: Re: [rfa] linespec.c, part 3 In-Reply-To: References: X-SW-Source: 2002-11/txt/msg00328.txt.bz2 David Carlton writes: > [ Sorry about that almost-empty message a few minutes ago. ] > > This part of the decode_line_1 refactoring moves some code > initializing some flags into a separate function set_flags. Some > comments: > Looks ok, but I have a question. Wouldn't the code that sets up has_comma and is_quote_enclosed also belong to this set_flags function? (Ideally a struct of flags could be set up). I see that has_comma plays the same trick with changing the char before parsing and restoring it afterwards. :-( I wonder if this could be changed by using a comma_ptr pointer and changing the for loop to continue until the comma_ptr instead of up to '\0'. Or just duplicate the first half of the string and play with that, it's not that we would be using a lot more memory, and it would seem less fragile than relying on a comment pleading you not to change 'ii'. Smae could be said for the current use of 'ii' with 'if'. But this is for a separate patch, you are just moving code around now. > * The current code sets a flag 'has_parens' to indicate whether or not > there is a paren, and a variable 'pp' to store the location of that > paren. This is redundant; we might as well have 'pp' indicate both > parts of that data, where pp is NULL if has_parens would be 0 and pp > isn't NULL if has_parens would be one. Given that I'd rather pass > as few variables by reference as possible, I decided to go that way; > I've named the resulting variable 'paren_pointer', and updated > references to either 'has_parens' or 'pp' accordingly. > good move. > * The current code uses the variable 'ii' both within the code that > I've extracted and elsewhere. But those uses are distinct: in fact, > the value of 'ii' is changed immediately after the code that this > patch extracts. > ok. go ahead, thanks! Elena > David Carlton > carlton@math.stanford.edu > > 2002-11-11 David Carlton > > * linespec.c (set_flags): New function. > (decode_line_1): Move code into set_flags. > > Index: linespec.c > =================================================================== > RCS file: /cvs/src/src/gdb/linespec.c,v > retrieving revision 1.27 > diff -u -p -r1.27 linespec.c > --- linespec.c 11 Nov 2002 21:18:55 -0000 1.27 > +++ linespec.c 11 Nov 2002 21:29:39 -0000 > @@ -42,6 +42,8 @@ extern char *operator_chars (char *, cha > static void initialize_defaults (struct symtab **default_symtab, > int *default_line); > > +static void set_flags (char *arg, int *is_quoted, char **paren_pointer); > + > static struct symtabs_and_lines decode_indirect (char **argptr); > > static void cplusplus_error (const char *name, const char *fmt, ...) ATTR_FORMAT (printf, 2, 3); > @@ -528,7 +530,7 @@ decode_line_1 (char **argptr, int funfir > struct symtabs_and_lines values; > struct symtab_and_line val; > register char *p, *p1; > - char *q, *pp, *ii, *p2; > + char *q, *ii, *p2; > #if 0 > char *q1; > #endif > @@ -542,10 +544,13 @@ decode_line_1 (char **argptr, int funfir > char *copy; > struct symbol *sym_class; > int i1; > + /* This is NULL if there are no parens in *ARGPTR, or a pointer to > + the closing parenthesis if there are parens. */ > + char *paren_pointer; > + /* This says whether or not something in *ARGPTR is quoted with > + completer_quotes (i.e. with single quotes). */ > int is_quoted; > int is_quote_enclosed; > - int has_parens; > - int has_if = 0; > int has_comma = 0; > struct symbol **sym_arr; > struct type *t; > @@ -563,45 +568,13 @@ decode_line_1 (char **argptr, int funfir > if (**argptr == '*') > return decode_indirect (argptr); > > - /* 'has_if' is for the syntax: > - * (gdb) break foo if (a==b) > - */ > - if ((ii = strstr (*argptr, " if ")) != NULL || > - (ii = strstr (*argptr, "\tif ")) != NULL || > - (ii = strstr (*argptr, " if\t")) != NULL || > - (ii = strstr (*argptr, "\tif\t")) != NULL || > - (ii = strstr (*argptr, " if(")) != NULL || > - (ii = strstr (*argptr, "\tif( ")) != NULL) > - has_if = 1; > - /* Temporarily zap out "if (condition)" to not > - * confuse the parenthesis-checking code below. > - * This is undone below. Do not change ii!! > - */ > - if (has_if) > - { > - *ii = '\0'; > - } > - > /* Set various flags. > - * 'has_parens' is important for overload checking, where > + * 'paren_pointer' is important for overload checking, where > * we allow things like: > * (gdb) break c::f(int) > */ > > - /* Maybe arg is FILE : LINENUM or FILE : FUNCTION */ > - > - is_quoted = (**argptr > - && strchr (get_gdb_completer_quote_characters (), > - **argptr) != NULL); > - > - has_parens = ((pp = strchr (*argptr, '(')) != NULL > - && (pp = strrchr (pp, ')')) != NULL); > - > - /* Now that we're safely past the has_parens check, > - * put back " if (condition)" so outer layers can see it > - */ > - if (has_if) > - *ii = ' '; > + set_flags (*argptr, &is_quoted, &paren_pointer); > > /* Maybe we were called with a line range FILENAME:LINENUM,FILENAME:LINENUM > and we must isolate the first half. Outer layers will call again later > @@ -682,7 +655,7 @@ decode_line_1 (char **argptr, int funfir > if (has_comma) > *ii = ','; > > - if ((p[0] == ':' || p[0] == '.') && !has_parens) > + if ((p[0] == ':' || p[0] == '.') && paren_pointer == NULL) > { > /* C++ */ > /* ... or Java */ > @@ -1075,9 +1048,9 @@ decode_line_1 (char **argptr, int funfir > if (p[-1] != '\'') > error ("Unmatched single quote."); > } > - else if (has_parens) > + else if (paren_pointer != NULL) > { > - p = pp + 1; > + p = paren_pointer + 1; > } > else > { > @@ -1214,6 +1187,46 @@ initialize_defaults (struct symtab **def > *default_symtab = cursal.symtab; > *default_line = cursal.line; > } > +} > + > +static void > +set_flags (char *arg, int *is_quoted, char **paren_pointer) > +{ > + char *ii; > + int has_if = 0; > + > + /* 'has_if' is for the syntax: > + * (gdb) break foo if (a==b) > + */ > + if ((ii = strstr (arg, " if ")) != NULL || > + (ii = strstr (arg, "\tif ")) != NULL || > + (ii = strstr (arg, " if\t")) != NULL || > + (ii = strstr (arg, "\tif\t")) != NULL || > + (ii = strstr (arg, " if(")) != NULL || > + (ii = strstr (arg, "\tif( ")) != NULL) > + has_if = 1; > + /* Temporarily zap out "if (condition)" to not > + * confuse the parenthesis-checking code below. > + * This is undone below. Do not change ii!! > + */ > + if (has_if) > + { > + *ii = '\0'; > + } > + > + *is_quoted = (*arg > + && strchr (get_gdb_completer_quote_characters (), > + *arg) != NULL); > + > + *paren_pointer = strchr (arg, '('); > + if (*paren_pointer != NULL) > + *paren_pointer = strrchr (*paren_pointer, ')'); > + > + /* Now that we're safely past the paren_pointer check, > + * put back " if (condition)" so outer layers can see it > + */ > + if (has_if) > + *ii = ' '; > } > >