From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17687 invoked by alias); 13 Mar 2009 22:10:33 -0000 Mailing-List: contact archer-help@sourceware.org; run by ezmlm Sender: Precedence: bulk List-Post: List-Help: List-Subscribe: List-Id: Received: (qmail 17674 invoked by uid 22791); 13 Mar 2009 22:10:32 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org To: Sami Wagiaalla Cc: Project Archer Subject: Re: [RFC] Koenig lookup patch References: <49BABABE.9080606@redhat.com> From: Tom Tromey Reply-To: Tom Tromey Date: Fri, 13 Mar 2009 22:10:00 -0000 In-Reply-To: <49BABABE.9080606@redhat.com> (Sami Wagiaalla's message of "Fri\, 13 Mar 2009 15\:57\:50 -0400") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2009-q1/txt/msg00382.txt.bz2 >>>>> "Sami" == Sami Wagiaalla writes: Sami> I have attached the patch for koenig lookup. I would like to hear your Sami> thoughts on the approach. Here's a few things I noticed. I stuck to the important stuff. I'll send another note describing the various formatting nits and similar trivia. Sami> space_identifier : '@' NAME Sami> + | '@' UNKNOWN_NAME Sami> { push_type_address_space (copy_name ($2.stoken)); Sami> push_type (tp_space_identifier); You have to duplicate the actions here. Sami> + if (sym == NULL && !lookup_minimal_symbol (tmp, NULL, NULL) && !is_a_field_of_this) lookup_minimal_symbol definitely won't follow the normal C++ lookup rules. So, this is not really implementing the C++ spec. I am not sure this can really be done in the lexer. I suppose you could look for a class member, based on the parser context, and return NAME in that case. I don't know whether that would always be correct. Another possible approach would be to have a C-specific evaluate function (other languages have examples of this, and the charset branch does it for C) that changes the function call code to do C++-specific stuff. Sami> + static char** prefix_array = NULL; Sami> + static int prefix_array_size = 0; Statics are usually bad. I guess this is to avoid adding extra arguments all up and down the evaluate_* call chain... Another approach would be to factor some of the code out into a helper function, then call it. Then things like that can simply be arguments. If you want to stick with the statics, you will have to add cleanups, as right now they can be leaked. Also I think you will have to change how they are allocated, because the value can be overwritten by a recursive call to evaluate_subexp_standard. Sami> + sym = lookup_symbol(concatenated_name, Sami> + exp->elts[pc + 1].block, VAR_DOMAIN, (int *) NULL); Sami> + Sami> + if (sym) Sami> + { Sami> + exp->elts[pc + 2].symbol = sym; Sami> + break; Sami> + } Sami> + } I suspect this kind of thing belongs elsewhere. My understanding is that ADL can add some symbols to the overload set. But, this seems to stop when it finds any matching symbol. Instead I would expect to find this in the code that computes the overload set. I see later there is a call to find_overload_match, but given that I don't understand the need to do a lookup_symbol here. Rewriting the expression in-place seems a bit weird. It may always work, but I am not certain ... I'm not sure whether a parsed expression can validly be re-evaluated in a context other than the one in which it was created. One thing I would like to see is a reasonably comprehensive set of tests, in particular including tests of whatever odd cases there are, and of some error cases. Tom