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

Fix detection of special case for constant evaluation #487

Merged
merged 3 commits into from
Jun 6, 2024

Conversation

aannleax
Copy link
Member

@aannleax aannleax commented Jun 4, 2024

We have special handling for expressions like ?x = 5, which uses the seek-function to quickly skip over values. However, the detection of this case was previously broken, so that we instead evaluated a = 5 function for each entry.

This PR should fix this problem and also has the observed performance benefits of #484.

@aannleax aannleax requested a review from matzemathics June 4, 2024 11:47
@aannleax aannleax force-pushed the fix/optimize-constants branch from 35a1e75 to 5f3eab2 Compare June 4, 2024 13:11
Copy link
Member

@monsterkrampe monsterkrampe left a comment

Choose a reason for hiding this comment

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

See comment above.

@aannleax aannleax requested a review from monsterkrampe June 6, 2024 07:45
Copy link
Member

@monsterkrampe monsterkrampe left a comment

Choose a reason for hiding this comment

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

Nice, thanks!
I'm wondering if there should also be a special case for parameters.len() == 0 but maybe we do not need to be concerned about this case. The current code should certainly cover this to a satisfying extent.

@mmarx
Copy link
Member

mmarx commented Jun 6, 2024

Nice, thanks! I'm wondering if there should also be a special case for parameters.len() == 0 but maybe we do not need to be concerned about this case.

Once we also handle that, I think it'll make sense to pull this functionality out into some generic piece of code called by the constructors – for now, the current approach is fine.

@aannleax aannleax merged commit 7f811d6 into main Jun 6, 2024
8 checks passed
@aannleax aannleax deleted the fix/optimize-constants branch June 6, 2024 08:41
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