-
Notifications
You must be signed in to change notification settings - Fork 3
Using the Maroon Stack for real. #43
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
Conversation
|
(returned with delays, all as expected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this PR - nice work on the stack machine refactor! This is a solid architectural move toward a more formal execution model.
What I like:
- The type-safe stack values are a big improvement over the previous runtime approach
- Stack variable count validation at compile time is clever
- The Retrn mechanism for proper call/return semantics is well thought out
- Good incremental approach - keeping the old code commented out during transition
Feedback:
- Those println! debug statements should probably be behind a debug flag or use proper logging before this goes to prod
- The commented code is helpful for now but we should plan to clean it up in follow-up PRs
- Minor: unused_validate_states_vec_or_panic function name suggests it should be removed?
Question: Is there a plan to migrate the other task types (Fibonacci, Divisors, etc.) to this new stack model? The current approach of only refactoring Factorial makes sense for proof-of-concept, but curious about the rollout strategy.
The test updates look good and the new execution model is much cleaner. The stack-based approach will definitely make it easier to reason about execution state.
LGTM with the minor cleanup suggestions!
|
Thanks! I'll merge this one for now, and try to find time this weekend to make the next batch of changes. Cool PRs from you too! |
No description provided.