From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19138 invoked by alias); 12 May 2010 22:21:34 -0000 Received: (qmail 19110 invoked by uid 22791); 12 May 2010 22:21:29 -0000 X-SWARE-Spam-Status: No, hits=-5.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 12 May 2010 22:21:20 +0000 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o4CMLIHk020567 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 12 May 2010 18:21:19 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx05.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o4CMLIeW030833; Wed, 12 May 2010 18:21:18 -0400 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id o4CMLHq1027277; Wed, 12 May 2010 18:21:17 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id DA7DCC88039; Wed, 12 May 2010 16:21:16 -0600 (MDT) From: Tom Tromey To: sami wagiaalla Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Test and support all cpp operator types References: <4BE9BE28.6080800@redhat.com> Reply-To: tromey@redhat.com Date: Thu, 13 May 2010 00:00:00 -0000 In-Reply-To: <4BE9BE28.6080800@redhat.com> (sami wagiaalla's message of "Tue, 11 May 2010 16:29:28 -0400") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-05/txt/msg00267.txt.bz2 >>>>> "Sami" == sami wagiaalla 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" 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