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

made maximum AST depth configurable #6893

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

firewave
Copy link
Collaborator

No description provided.

@firewave firewave marked this pull request as ready for review October 10, 2024 13:40
@chrchr-github
Copy link
Collaborator

What is the use case for this?

@firewave
Copy link
Collaborator Author

What is the use case for this?

Getting rid of "hidden" thresholds and bailouts (and too many branches).

There is also something going on with TestTokenizerCompileLimits I still want to look into and improve testing. This would allow testing with different limits and even simplify the current example.

@danmar
Copy link
Owner

danmar commented Oct 10, 2024

I don't really see the idea right now neither. If the constant is only used in 1 file it does not hurt that it is defined there.

you also made it non-const and there is no good reason for that right now.

I don't feel convinced that this should be a runtime configuration option.

@firewave firewave marked this pull request as draft October 10, 2024 14:31
@firewave
Copy link
Collaborator Author

The current limit is way too high and still might have significant impact on performance. So some people might need to lower that. Making it configurable will make it easier to test for a more reasonable value.

But the threshold might mot be helpful because in case it will hit something beforehand will already have consumed a considerable amount of time. I encountered that while trying to make TestTokenizerCompileLimits pass in a reasonable amount of time in #6587. I have not looked into it yet but it probably requires some additional threshold and that is probably one that should be in sync with the one pulled out here.

Sorry for forgetting to mention this beforehand.

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