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

[Tom Tromey <tromey at redhat dot com>]

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

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