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

[refactor] Improve the Display impl for ProgramCore #2532

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

ljedrz
Copy link
Collaborator

@ljedrz ljedrz commented Aug 22, 2024

This is technically a refactoring, but with some performance implications; this impl also shouldn't cause a panic anymore in case of a formatting error.

Found via fuzzing, with Stack::new causing multiple crashes during the parse check.

Copy link
Collaborator

@vicsn vicsn left a comment

Choose a reason for hiding this comment

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

Seems harmless if tests pass

@zosorock
Copy link
Contributor

The same CI jobs have failed 3 times. @vicsn can you let us know if you feel these are relevant or not, please?

@vicsn
Copy link
Collaborator

vicsn commented Oct 28, 2024

Merging upstream should do the trick for fmt, @ljedrz let me know if you can tackle it otherwise I'll clone this PR to fix it!

The parameters test is unrelated, not sure why it fails, will look into it if it becomes a recurring problem.

@ljedrz
Copy link
Collaborator Author

ljedrz commented Oct 28, 2024

@vicsn sure, I'll rebase shortly 👌

@ljedrz ljedrz force-pushed the refactor/display_programcore branch from c7e8686 to 93b4aca Compare October 28, 2024 11:41
Copy link
Contributor

@zkxuerb zkxuerb left a comment

Choose a reason for hiding this comment

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

LGTM

@alzger alzger merged commit ecddac5 into ProvableHQ:staging Nov 4, 2024
84 checks passed
@zosorock zosorock added the enhancement New feature or request label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants