Re: [PATCH] Test and support all cpp operator types

Tom Tromey
2010-05-12T22:21:34Z

Sami> This patch adds testing and support for the following types of operator
Sami> lookup:
Sami> - non-member operators.
Sami> - imported operators (using directive, anonymous namespaces).
Sami> - ADL operators.

I like the general approach of this patch.

I have a few comments.

Sami> @@ -800,21 +800,27 @@ make_symbol_overload_list_using (const char *func_name,
Sami> const char *namespace)
[...]
Sami> - for (current = block_using (get_selected_block (0));
Sami> - current != NULL;
Sami> - current = current->next)
Sami> - {
Sami> - if (strcmp (namespace, current->import_dest) == 0)
Sami> - {
Sami> - make_symbol_overload_list_using (func_name,
Sami> - current->import_src);
Sami> - }
Sami> - }
Sami> + for (block = get_selected_block (0);
Sami> + block != NULL;
Sami> + block = BLOCK_SUPERBLOCK(block))
Sami> + for (current = block_using (block);
Sami> + current != NULL;
Sami> + current = current->next)
Sami> + {
[...]

This part seems a little weird to me.
make_symbol_overload_list_using calls make_symbol_overload_list_namespace,
which calls make_symbol_overload_list_qualified, which itself
starts with get_selected_block and iterates over the superblocks.

It seems to me that only one such iteration should be needed.
I don't have a test case but it seems like this could cause incorrect
results in some corner case.

Also, missing space after BLOCK_SUPERBLOCK.

Sami> + /* If this is a namespace alias or imported declaration ignore it. */
Sami> + if (current->alias != NULL || current->declaration !=NULL)

Missing space before NULL.

Sami> + if (strcmp (namespace, current->import_dest) == 0)
Sami> + make_symbol_overload_list_using (func_name, current->import_src);
Sami> +
Sami> + }

Wrong indentation on the line after the 'if'.
Gratuitous blank line before the closing brace.

Sami> +# some unary operators for good measure
Sami> +# Cannot resolve function operator++ to any overloaded instance
Sami> +gdb_test "p ++q" "= 30"

It would be interesting to know if "q++" would call an overloaded
postfix operator++. These have a hack in the C++ spec to differentiate
them from prefix ++.

I'm also curious to know if "ADL avoidance" works properly when a
qualified reference to "operator<whatever>" is used. I didn't see a
test for that in this patch. (If there is already one in the test
suite, then of course we don't need a new one...)

Sami> +++ b/gdb/testsuite/gdb.cp/operator.exp
[...]
Sami> +set prms_id 0
Sami> +set bug_id 0

I think Joel just nuked these from the rest of the test suite.

Sami> +set testfile operator
Sami> +set srcfile ${testfile}.cc
Sami> +set binfile ${objdir}/${subdir}/${testfile}
Sami> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug c++}] != "" } {
Sami> + untested "Couldn't compile test program"
Sami> + return -1
Sami> +}
Sami> +
Sami> +# Get things started.
Sami> +
Sami> +gdb_exit
Sami> +gdb_start
Sami> +gdb_reinitialize_dir $srcdir/$subdir
Sami> +gdb_load ${binfile}

There is some convenience proc that automates a lot of this now.
I forget what but I think it is on the wiki.

Sami> \ No newline at end of file

Please add one.

Sami> +static struct value *
Sami> +value_user_defined_cpp_op (struct value **args, int nargs, char *operator,
Sami> + int *static_memfuncp)
[...]
Sami> + find_overload_match (arg_types, nargs, operator, 2 /* could be method */,

This should use the new enum constant, not '2'.

I think this patch ought to update all other callers of
find_overload_match to use the constants.

Sami> +/* Lookup user defined operator NAME. Return a value representing the
Sami> + function, otherwise return NULL. */
Sami> +
Sami> +static struct value *
Sami> +value_user_defined_op (struct value **argp, struct value **args, char *name,
Sami> + int *static_memfuncp, int nargs)

I think the error return here is odd.
value_struct_elt will throw, rather than return NULL.
Perhaps that is what value_user_defined_cpp_op ought to do as well.

Sami> @@ -2266,7 +2266,6 @@ value_find_oload_method_list (struct value **argp, const char *method,
Sami> struct type *t;

Sami> t = check_typedef (value_type (*argp));
Sami> -
Sami> /* Code snarfed from value_struct_elt. */

Don't change whitespace.

Sami> + METHOD can be one of three valuse:

Typo, "values".

I haven't yet digested the rest of the changes to find_overload_match.

Sami> +enum oload_search_type { NON_METHOD, METHOD, BOTH };
Sami> extern int find_overload_match (struct type **arg_types, int nargs,
Sami> - const char *name, int method, int lax,
Sami> + const char *name,
Sami> + enum oload_search_type method, int lax,
Sami> struct value **objp, struct symbol *fsym,
Sami> struct value **valp, struct symbol **symp,
Sami> int *staticp, const int no_adl);

Blank line after the enum definition.
Normally I would insist on a comment for the enum as well, but I think
it is reasonably clear, especially given the intro comment to
find_overload_match.

Tom