feat(forth3): return interpreter actions from builtins#65
Conversation
✅ Deploy Preview for merry-scone-cc7a60 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| Execute, | ||
| #[derive(Debug, Copy, Clone, Eq, PartialEq)] | ||
|
|
||
| pub enum InterpretAction { |
There was a problem hiding this comment.
I'm not particularly attached to the name InterpretAction, but I couldn't think of something better --- @jamesmunns, if you want to bikeshed this, I'm all ears!
There was a problem hiding this comment.
I don't have a very strong opinion on the name, though it would probably be good to add doc comments here that explain what each choice means.
For AcceptInput (which I think means "block until input buffer is filled"), do we want to make this something like:
pub enum InterpretAction {
Done,
Continue,
BlockedOn(BlockReason),
}
pub enum BlockReason {
AcceptInput,
OutputFull,
// ...
Custom(u32), // User defined blocking reason?
}There was a problem hiding this comment.
whoops, I meant to add docs to this --- thanks for calling that out!
There was a problem hiding this comment.
we could add a nested enum of blocking reasons, but the one you're proposing does make the return value substantially larger: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2a13f71d45fd5a02872bd972c1ecba58
worrying about enum size here may be a bit of a premature optimization, but it is the return value of basically every interpreter step, so we pass these enums around kind of a lot --- and they're already inside of a Result<InterpretAction, Error>, which adds a couple more layers of discriminants...we probably want to ensure that the return value of these functions fits in a register, ideally?
This branch changes the `forth3` API to prepare for Forth-driven IO. In particular, builtin words and VM execution steps are changed from returning a `Ok(())` branch when the builtin/execution step succeeds to returning a new `InterpretAction` type in the `Ok` branch, which determines what action the host should perform next. `InterpretAction` is an enum which currently has three variants: - `InterpretAction::Done`, which has the same semantics that returning `Ok(())` currently has: if returned from a builtin, it indicates that the VM should pop the current callstack frame and keep executing, and if returned by the VM to the host, it indicates that the VM's callstack is empty. - `InterpretAction::Continue`, which replaces the `Error::PendingCallAgain` variant (it seemed more morally correct for this to be an `Ok` return rather than an `Err`), and indicates that the VM should continue executing what's currently on the stack when returned from a builtin - `InterpretAction::AcceptInput`, which indicates that the VM needs to wait for the host to fill its input buffer. In the future, `InterpretAction::AcceptInput` will be used to provide an improved abstraction for communicating to the host that the VM is requesting IO. We may also add a variant indicating that the VM needs the host to drain its output buffer. When we add implementations of Forth words such as `quit`, which fill the input buffer, they will return the `AcceptInput` variant, allowing us to request IO from builtins provided by `forth3` without requiring them to actually *do* the IO. This lets us avoid making the entire VM generic over some `Read` and `Write` traits (or `AsyncRead`/`AsyncWrite` traits), which don't exist in no-std, by temporarily breaking out of a VM's run loop and returning to the host when I/O is needed.
hawkw
force-pushed
the
eliza/interpretaction
branch
from
b9acd48 to
538edcc
Compare
jamesmunns
left a comment
jamesmunns
left a comment
There was a problem hiding this comment.
Overall looks good to me. Some thoughts on the blocking enum, and I'd like to have docs for InterpretAction, but in principle I'm very +1 for this!
|
@hawkw does it make sense to get this merged sometime? |
2 participants
This branch changes the
forth3API to prepare for Forth-driven IO. Inparticular, builtin words and VM execution steps are changed from
returning a
Ok(())branch when the builtin/execution step succeeds toreturning a new
InterpretActiontype in theOkbranch, whichdetermines what action the host should perform next.
InterpretActionis an enum which currently has three variants:InterpretAction::Done, which has the same semantics that returningOk(())currently has: if returned from a builtin, it indicates thatthe VM should pop the current callstack frame and keep executing, and
if returned by the VM to the host, it indicates that the VM's
callstack is empty.
InterpretAction::Continue, which replaces theError::PendingCallAgainvariant (it seemed more morally correct forthis to be an
Okreturn rather than anErr), and indicates thatthe VM should continue executing what's currently on the stack when
returned from a builtin
InterpretAction::AcceptInput, which indicates that the VM needs towait for the host to fill its input buffer.
In the future,
InterpretAction::AcceptInputwill be used to provide animproved abstraction for communicating to the host that the VM is
requesting IO. We may also add a variant indicating that the VM needs
the host to drain its output buffer. When we add implementations of
Forth words such as
quit, which fill the input buffer, they willreturn the
AcceptInputvariant, allowing us to request IO frombuiltins provided by
forth3without requiring them to actually dothe IO. This lets us avoid making the entire VM generic over some
Readand
Writetraits (orAsyncRead/AsyncWritetraits), which don'texist in no-std, by temporarily breaking out of a VM's run loop and
returning to the host when I/O is needed.