This is the mail archive of the mailing list for the Archer project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC] Koenig lookup patch

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

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.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]