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 optional trace to CairoVM struct and add enable trace flag #23

Closed
wants to merge 0 commits into from

Conversation

lambda-0x
Copy link
Contributor

Fixes: #9

I am new to zig so let me know if something is non idiomatic or there is a better way to do it.

Copy link
Collaborator

@AbdelStark AbdelStark left a comment

Choose a reason for hiding this comment

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

left some comments

src/vm/core.zig Outdated Show resolved Hide resolved
src/vm/core.zig Outdated
}
}

// TODO: Find a valid Instruction so we can call `runInstruction` to check if traces
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove commented test and create an issue with the content of this test instead please

Copy link
Collaborator

@AbdelStark AbdelStark left a comment

Choose a reason for hiding this comment

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

I am wondering if we should just make the config struct of the cmd in a config.zig file and share it between the cli and the core vm.
Instead of passing individual flags like we do in this PR.
I am worried that it will be harder to maintain if each time we add a new flag in the init function and a new field in the VM.
For example we see here that you had to change each individual test to add the new parameter.
If it was the struct and if the struct has a default function you would not have to change the tests most of the time, only when you need to change the config for an actual test.

@lambda-0x
Copy link
Contributor Author

reopened: #27

Copy link

Pull Request closed and locked due to lack of activity.
If you'd like to build on this closed PR, you can clone it using this method: https://stackoverflow.com/a/14969986
Then open a new PR, referencing this closed PR in your message.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add optional trace to CairoVM struct and add enable trace flag
2 participants