Re: [RFC] Koenig lookup patch 3

[Tom Tromey <tromey@redhat.com>]

>>>>> "Sami" == Sami Wagiaalla <swagiaal@redhat.com> writes:

Sami> My previous patch was wrapped by my mail agent. Here is a correct one:
Sami> 2009-10-08  Sami Wagiaalla  <swagiaal@redhat.com>

Looking much nicer!

Sami> +exp	:	adl_func '('
Sami> +adl_func	:	UNKNOWN_NAME

It seems like these additions would introduce a parser ambiguity,
because UNKNOWN_NAME could be parsed as adl_func or as name_not_typename
(and hence variable).

Is bison silent about this?

Sami> +static void make_symbol_overload_list_namespace (const char *func_name,
Sami> +                                                 const char *namespace);
Sami> +
Sami> +static void make_symbol_overload_list_adl_namespace (struct type *type,
Sami> +                                                     const char *func_name);

GDB does this all over, but in new code we're now preferring that
functions be ordered so that prototypes aren't necessary for static
functions.  This is a bit simpler to maintain.

Sami> +make_symbol_overload_list_adl_namespace (struct type *type, const char *func_name )

Line wraps.  Space before close paren.

Sami> +  char* namespace;
Sami> +  char* type_name;

"char *".

Sami> +    return make_symbol_overload_list_adl_namespace(TYPE_TARGET_TYPE (type), func_name);

Line wraps (?).  Also, space before open paren.  There are several of
these.

Sami> +    case OP_ADL_FUNC:
Sami>      case OP_VAR_VALUE:

I don't understand the placement of the new case here.
How do we ever end up in the new code here, which seems to be in the
OP_FUNCALL case?

Sami> +  /* Check public base type */
Sami> +  if (TYPE_CODE(type) == TYPE_CODE_CLASS)
Sami> +    for (i = 0; i < TYPE_N_BASECLASSES (type); i++)
Sami> +      {
Sami> +        if(BASETYPE_VIA_PUBLIC (type, i))

Why the check for public?

Sami> +          func_name = (char*) alloca (name_len+1);

Spaces around "+".  I think I saw this in more than one place.


Sami> +          find_overload_match (arg_types, nargs, func_name,
Sami> +                              0 /* not method */ , 0 /* strict match */ ,
Sami> +                              NULL, NULL /* pass NULL symbol to signal ADL lookup */ ,

If this is a new convention for the find_overload_match argument, then
it ought to be documented in that function's introductory comment.

Sami> +    /* OP_ADL_FUNC specifies that the argument is to be looked up in an
Sami> +       Argument Dependent manner (keonig lookup) */

Typo, and capitalize "Koenig".

Sami> @@ -2108,12 +2108,25 @@ find_overload_match (struct type **arg_types, int nargs,
Sami> +              old_cleanups = make_cleanup (xfree, func_name);
Sami> -      old_cleanups = make_cleanup (xfree, func_name);
Sami>        make_cleanup (xfree, oload_syms);
Sami>        make_cleanup (xfree, oload_champ_bv);

I think the cleanup logic here is wrong now.
It seems like the code can make a cleanup but still have old_cleanups==NULL.
This will leave dangling cleanups; the usual rule (for functions not
return a cleanup or making one as a side effect) is that local cleanups
must always be run before function exit.

One way to do this would be to initialize old_cleanups as a null cleanup
early on.  Then, never set it elsewhere in the function, and change the
do_cleanups call at the end to be unconditional.  This is a typical
idiom elsewhere in gdb.

Sami> +  if (!searched_deeper)
Sami> +      make_symbol_overload_list_adl(arg_types, nargs, func_name);

Indentation.

I like the new test cases.  I did not go over them extensively, but I
have two points to make.  One is, I think there should probably be some
tests for nested namespaces.  The other is that it would be nice to be
assured that each supported case has a corresponding test; including
tests for things like overload resolution picking the best match both
when the best match is found via ADL and when it is not.

Tom