Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazy compile in lambda #219

Merged
merged 14 commits into from
Dec 23, 2021
Merged

Lazy compile in lambda #219

merged 14 commits into from
Dec 23, 2021

Conversation

ealeykin
Copy link
Contributor

Lazy compile delegate in Lambda so the internal Expression can be processed in custom way without default compile overhead.

@ealeykin
Copy link
Contributor Author

@davideicardi @metoule As a workaround to fix #207 could we merge in this lazy delegate compile, so I can use raw expression without default delegate compile overhead?

@metoule
Copy link
Contributor

metoule commented Dec 23, 2021

That's a good first step, that has minimal impact on the current interface. However, that alone won't fix #207 because the root cause is the UsedParameters vs DeclaredParameters.
Maybe you can also backport the DetectUseParameters option?

I think maybe it should be set to false by default but that is potentially a major change.

PS. I'm currently on holidays, with minimal access to a computer, so I won't be able to push commits for a few days.

@ealeykin
Copy link
Contributor Author

ealeykin commented Dec 23, 2021

I won't use Lambda.Invoke. Since the exception highlighted in #207 comes from compile in Lambda constructor you can do the following:

// parameters
var parameters = ...;

// parse
var lambda = interpreter.SetExpression(innerExpression).Parse(expressionText, parameters);

// custom compile and pass all required parameters myself
var delegate = Expression.Lambda(lambda.Expression, parameters).Compile();

// invoke 
var result = delegate.DynamicInvoke(....)

Am I missing something?

@metoule
Copy link
Contributor

metoule commented Dec 23, 2021

In that case, the lazy compilation is enough :)

Copy link
Contributor

@metoule metoule left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@davideicardi davideicardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you very much @halamah !

I will merge this and release a new version.
But let's keep #211 and related PRs open, because I think that your original idea is the right solution for the future.

@davideicardi davideicardi merged commit 1d01401 into dynamicexpresso:master Dec 23, 2021
@davideicardi
Copy link
Member

Published v2.10.0! Thank you again @halamah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants