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.

Sign up for GitHub

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

Jump to bottom

Implement HM 6 #5

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Implement HM 6 #5

wants to merge 6 commits into from

Conversation

rkhudov
Copy link
Owner

@rkhudov rkhudov commented Nov 3, 2023

No description provided.

@rkhudov rkhudov requested a review from kinnison November 3, 2023 16:40
kinnison
kinnison reviewed Nov 15, 2023
View reviewed changes
Copy link
Collaborator

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

I think there's a couple of iffy API/storage decisions but otherwise this looks good.

When you've thought about my comments and made a decision, let me know, and I'll break out my test suite to see how this fares against the test cases I use.

btf_interp/src/lib.rs Outdated Show resolved Hide resolved
btf_interp/src/lib.rs Outdated Show resolved Hide resolved
btf_types/src/lib.rs Show resolved Hide resolved
rkhudov
rkhudov commented Nov 16, 2023
View reviewed changes
src/main.rs
VirtualMachine::new(&bf_program, args.cells, args.extensible);
vm.interpreter();
let _ = vm.interpret(&mut stdin(), &mut stderr());
Copy link
Owner Author

Choose a reason for hiding this comment

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

FYI, I test my program using this website: https://minond.xyz/brainfuck/

Copy link
Owner Author

Choose a reason for hiding this comment

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

Should be fine now

@rkhudov rkhudov requested a review from kinnison November 16, 2023 11:22
kinnison
kinnison requested changes Nov 26, 2023
View reviewed changes
Copy link
Collaborator

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

Everything looks nice except that the extensible tape doesn't work which means if I try some of my tests with a 1 cell extensible tape it fails.

btf_interp/src/lib.rs Show resolved Hide resolved
@rkhudov rkhudov requested a review from kinnison November 27, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kinnison kinnison Awaiting requested review from kinnison

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rkhudov @kinnison