Skip to content

made maximum AST depth configurable#6893

Draft
firewave wants to merge 1 commit into
cppcheck-opensource:mainfrom
firewave:compilelimit
Draft

made maximum AST depth configurable#6893
firewave wants to merge 1 commit into
cppcheck-opensource:mainfrom
firewave:compilelimit

Conversation

@firewave
Copy link
Copy Markdown
Collaborator

No description provided.

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

What is the use case for this?

@firewave
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
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