This is the mail archive of the
archer@sourceware.org
mailing list for the Archer project.
Re: [RFC] Koenig lookup patch 3
- From: Tom Tromey <tromey at redhat dot com>
- To: Sami Wagiaalla <swagiaal at redhat dot com>
- Cc: Project Archer <archer at sourceware dot org>
- Date: Thu, 15 Oct 2009 14:48:34 -0600
- Subject: Re: [RFC] Koenig lookup patch 3
- References: <49BABABE.9080606@redhat.com> <m3d4clrs4z.fsf@fleche.redhat.com><49F87751.8050405@redhat.com> <m3iqkndmck.fsf@fleche.redhat.com><4A969900.6040100@redhat.com> <m3zl9lorki.fsf@fleche.redhat.com><4AD6340F.9070108@redhat.com> <4AD63BA4.4070309@redhat.com>
- Reply-to: Tom Tromey <tromey at redhat dot 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