From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7302 invoked by alias); 17 Mar 2009 18:39:45 -0000 Mailing-List: contact archer-help@sourceware.org; run by ezmlm Sender: Precedence: bulk List-Post: List-Help: List-Subscribe: List-Id: Received: (qmail 7292 invoked by uid 22791); 17 Mar 2009 18:39:44 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Message-ID: <49BFEE44.7080901@redhat.com> Date: Tue, 17 Mar 2009 18:39:00 -0000 From: Sami Wagiaalla User-Agent: Thunderbird 2.0.0.17 (X11/20081009) MIME-Version: 1.0 To: Tom Tromey CC: Project Archer Subject: Re: [RFC] Koenig lookup patch References: <49BABABE.9080606@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2009-q1/txt/msg00387.txt.bz2 > 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. > Hmm it looks like NAME is no longer nessesary 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. > This is done to distinguish an adl_func from names which are meant to be found through lookup_minimal_symbol. Without this check tests for checkpoint try to call 'lseek', this gets wrongly reduced to an adl_function and the later ADL lookup fails. > 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. > Yeah, the above code can actually be moved into find_overload_match I never really thought of it that way. This will get rid of the statics, and allocation problems. > Rewriting the expression in-place seems a bit weird. It makes sense to me. You see the parameters of this expression actually depend on the result of the evaluation of the parameters particularly of the parameters are expressed in complex expressions and not plain variable names. > 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. > If by context you mean scope, that is preserved and passed in as part of the "pre-expression" here exp->elts[pc + 1].block. I cant think of anything else but we can adapt this when we run into such cases. > > 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. Looks like I forgot to git add my tests. Please see: git show fbf9dc5c8c817b100ad7ce5c680c4bf346e326df Thanks for the review. I'll post an update version soon. Sami > > Tom