Re: [patch 0/3] Re: [RFA] c++/11734 revisited (and c++/12273)

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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;
> +}
> +
>  \f
>  /* 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