Re: [RFC] Koenig lookup patch

[Sami Wagiaalla <swagiaal@redhat.com>]

> 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