Re: [RFC] Koenig lookup patch

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Sami" == Sami Wagiaalla <swagiaal@redhat.com> 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