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

feat: add max recursion depth param to avoid hydration stack overflow #74

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

MartianGreed
Copy link
Contributor

… stack overflow

@MartianGreed MartianGreed force-pushed the feat/hydration-max-depth branch from 0f7b9b7 to cc73ad4 Compare December 4, 2024 10:37
pub fn hydrate(
token: Self,
filtered: &HashMap<String, Token>,
_recursion_max_depth: usize,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why _? If they are used, we shouldn't add this leading _.

Comment on lines +1145 to +1146
#[test]
fn test_collect_tokens() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind moving this test out of this file since it's becoming huge? in this case having a parser_test.rs for now should suffice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or use include_str! and put the Sierra file inside a /test_data folder.

@MartianGreed MartianGreed force-pushed the feat/hydration-max-depth branch from cc73ad4 to de92e0f Compare December 4, 2024 16:27
@glihm glihm changed the title feat: add max recursion depth param to avoid recursing types to cause… feat: add max recursion depth param to avoid hydration stack overflow Dec 4, 2024
@glihm glihm merged commit 6fefd8b into cartridge-gg:main Dec 4, 2024
3 checks passed
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.

2 participants