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

Jan Kratochvil
2011-03-13T22:28:40Z

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