From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19628 invoked by alias); 13 Mar 2011 22:28:40 -0000 Received: (qmail 19618 invoked by uid 22791); 13 Mar 2011 22:28:39 -0000 X-SWARE-Spam-Status: No, hits=-5.5 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_CP,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 13 Mar 2011 22:28:29 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p2DMSSkK011725 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 13 Mar 2011 18:28:28 -0400 Received: from host1.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p2DMSQT9010809 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 13 Mar 2011 18:28:27 -0400 Received: from host1.jankratochvil.net (localhost [127.0.0.1]) by host1.jankratochvil.net (8.14.4/8.14.4) with ESMTP id p2DMSQLh011840; Sun, 13 Mar 2011 23:28:26 +0100 Received: (from jkratoch@localhost) by host1.jankratochvil.net (8.14.4/8.14.4/Submit) id p2DMSPM4011838; Sun, 13 Mar 2011 23:28:25 +0100 Date: Mon, 14 Mar 2011 07:52:00 -0000 From: Jan Kratochvil To: Keith Seitz Cc: gdb-patches@sourceware.org Subject: Re: [patch 0/3] Re: [RFA] c++/11734 revisited (and c++/12273) Message-ID: <20110313222824.GA24322@host1.jankratochvil.net> References: <20110227211637.GA18378@host1.dyn.jankratochvil.net> <4D6D6C74.8080304@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4D6D6C74.8080304@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-03/txt/msg00706.txt.bz2 On Tue, 01 Mar 2011 23:00:20 +0100, Keith Seitz wrote: [...] > --- linespec.c 1 Mar 2011 00:26:14 -0000 1.110 > +++ linespec.c 1 Mar 2011 21:38:20 -0000 [...] > @@ -663,6 +678,96 @@ find_method_overload_end (char *p) > > return p; > } > + > +/* A helper function for decerning whether the string P contains > + overload information. */ > + > +static int > +is_overloaded (char *p) > +{ > + /* Locate a parenthesis in P. */ > + char *paren = strchr (p, '('); > + > + if (paren != NULL) > + { > + /* Locate a (possible) "if" clause. */ > + char *has_if = strstr (p, "if"); > + > + /* Double-check that the "if" was not part of some sort of name, > + by checking the characters before and after the "if". */ > + if (has_if > p && (isalnum (*(has_if - 1)) || isalnum (*(has_if + 2)))) Although I wrote `isalnum' it is a wrong character class, for example isalnum ('_') == 0 but `_' is a valid identifier character. Rather than examining is_overloaded in detail suggesting to rather drop this whole function. Please see its callers. > + has_if = NULL; > + > + /* If the "if" was seen after the parenthesis, P points to > + an overloaded method. */ > + if (has_if > paren || has_if == NULL) > + return 1; > + } > + > + /* Otherwise, P does not point to an overloaded method. */ > + return 0; > +} > + > + > +/* Does P point to a sequence of characters which implies the end > + of a name? Terminals include "if" and "thread" clauses. */ > + > +static int > +name_end (char *p) > +{ > + while (isspace (*p)) > + ++p; > + if (*p == 'i' && *(p + 1) == 'f' > + && (isspace (*(p + 2)) || *(p + 2) == '\0' || *(p + 2) == '(')) wrt '(' - it did not work in pre-physname GDB as a whitespace is required by find_condition_and_thread. But it was present there in `set_flags' so I agree it may be safer to keep it here. Just a statement, no request for any change. > + return 1; > + > + if (strncmp (p, "thread", 6) == 0 > + && ((isspace (*p + 6)) || *(p + 6) == '\0')) All these cases should be written like p[6] both for the IMO better readability and for making it less fragile against bugs like `(*p + 6)'. > + return 1; There may be also "task" catching but that is used by Ada and it already worked before without such exception so it is probably OK. Just a statement, no request for any change. > + > + return 0; > +} > + > +/* Keep important information used when looking up a name. This includes > + template parameters, overload information, and important keywords. */ > + > +static char * > +keep_name_info (char *ptr) > +{ > + char *p = ptr; > + > + /* Keep any template parameters. */ > + if (name_end (ptr)) > + goto done; > + > + while (isspace (*p)) > + ++p; > + if (*p == '<') > + ptr = p = find_template_name_end (ptr); > + > + if (name_end (ptr)) > + goto done; > + > + /* Keep method overload information. */ > + if (is_overloaded (p)) It seems to me here could be sufficient instead of is_overloaded just: if (*p == '(') Or do you have a countercase where it would not work? > + ptr = p = find_method_overload_end (p); > + > + if (name_end (ptr)) > + goto done; > + > + /* Keep important keywords. */ > + while (isspace (*p)) > + ++p; > + if (strncmp (p, "const", 5) == 0 > + && (isspace (*(p + 5)) || *(p + 5) == '\0' || *(p + 5) == '\'')) *(p + 5) == '\'' -> strchr (get_gdb_completer_quote_characters (), p[5]) != NULL > + ptr = p = p + 5; > + > + done: > + while (isspace (*(ptr - 1))) > + --ptr; Underrun of the strings you reported as present even in pre-physname GDB so I have just filed it as: decode_linespec_1 vagrind errors on "" http://sourceware.org/bugzilla/show_bug.cgi?id=12535 > + return ptr; > +} > + > > /* The parser of linespec itself. */ > [...] > @@ -1470,6 +1585,10 @@ decode_compound (char **argptr, int funf > /* We couldn't find a class, so we're in case 2 above. We check the > entire name as a symbol instead. */ > > + if (current_language->la_language == language_cplus > + || current_language->la_language == language_java) > + p = keep_name_info (p); Wrong indentation. > + > copy = (char *) alloca (p - saved_arg2 + 1); > memcpy (copy, saved_arg2, p - saved_arg2); > /* Note: if is_quoted should be true, we snuff out quote here [...] > @@ -1594,20 +1722,32 @@ find_method (int funfirstline, char ***c > /* If we were given a specific overload instance, use that > (or error if no matches were found). Otherwise ask the user > which one to use. */ > - if (strchr (saved_arg, '(') != NULL) > + if (is_overloaded (saved_arg)) It seems to me here could be sufficient instead of is_overloaded just: if (strchr (copy, '(') != NULL) Or do you have a countercase where it would not work? > { > int i; > - char *name = saved_arg; > - char *canon = cp_canonicalize_string (name); > + char *name; > + char *canon; > struct cleanup *cleanup; > > + /* Construct the proper search name based on SYM_CLASS and COPY. > + SAVED_ARG may contain a valid name, but that name might not be > + what is actually stored in the symbol table. For example, > + if SAVED_ARG (and SYM_CLASS) were found via an import > + ("using namespace" in C++), then the physname of > + SYM_CLASS ("A::myclass") may not be the same as SAVED_ARG > + ("myclass"). */ > + name = xmalloc (strlen (SYMBOL_NATURAL_NAME (sym_class)) > + + 2 /* "::" */ + strlen (copy) + 1); > + strcpy (name, SYMBOL_NATURAL_NAME (sym_class)); > + strcat (name, "::"); > + strcat (name, copy); > + canon = cp_canonicalize_string (name); > if (canon != NULL) > { > + xfree (name); > name = canon; > - cleanup = make_cleanup (xfree, canon); > } > - else > - cleanup = make_cleanup (null_cleanup, NULL); > + cleanup = make_cleanup (xfree, name); > > for (i = 0; i < i1; ++i) > { [...] It is approved with these changes if you agree with them. I do not expect anyone else is going to futher review it. Thanks, Jan