-
Notifications
You must be signed in to change notification settings - Fork 44
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
Faster execution and a new pseudo-instruction #273
Conversation
… IR (consistent with textbook). Change ALU control signals to be consistent with textbook. Change control signals in SkipCond to reflect the PC increment step rather than the ALU test.
… delay of 10ms is used, makes some programs far too slow)
Can be used to load the address of a label (less confusing than abusing JnS for that purpose)
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.
Looks all good. Just one quick question.
var realDelay = delay; | ||
var itersPerLoop = 1; | ||
if (realDelay < 10) { | ||
realDelay = 10; |
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.
What is this realDelay
variable used for?
I don't see it used anywhere else, unless its used to replace the delay
variable inside the setInterval
function
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.
Of course that was the plan... Doesn't make any difference though because JS seems to assume a minimum of 10 ms anyway. I've fixed this (see commit below). I've also added a fix for a travis ci problem.
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.
Looks all good! 💯 👍 🚀 🎉
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.
LGTM
PR has been approved. Version also increment to v1.2.0 |
This pull request contains some minor UI fixes, and two bigger changes: