From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18439 invoked by alias); 15 Oct 2009 20:48:46 -0000 Mailing-List: contact archer-help@sourceware.org; run by ezmlm Sender: Precedence: bulk List-Post: List-Help: List-Subscribe: List-Id: Received: (qmail 18393 invoked by uid 22791); 15 Oct 2009 20:48:44 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org From: Tom Tromey To: Sami Wagiaalla Cc: Project Archer Subject: Re: [RFC] Koenig lookup patch 3 References: <49BABABE.9080606@redhat.com> <49F87751.8050405@redhat.com> <4A969900.6040100@redhat.com> <4AD6340F.9070108@redhat.com> <4AD63BA4.4070309@redhat.com> Reply-To: Tom Tromey Date: Thu, 15 Oct 2009 20:48:00 -0000 In-Reply-To: <4AD63BA4.4070309@redhat.com> (Sami Wagiaalla's message of "Wed, 14 Oct 2009 16:59:16 -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 X-SW-Source: 2009-q4/txt/msg00014.txt.bz2 >>>>> "Sami" == Sami Wagiaalla writes: Sami> My previous patch was wrapped by my mail agent. Here is a correct one: Sami> 2009-10-08 Sami Wagiaalla 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