-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
refactor(core): convert core.print() to a builtin op #10436
Conversation
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.
Lovely. LGTM.
let (msg, is_err) = args; | ||
if is_err { | ||
eprint!("{}", msg); | ||
stdout().flush().unwrap(); |
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.
Shouldn't it be stderr().flush().unwrap()
?
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.
I think you're right, but it's not 100% clear if the original intention was to flush stdout
when writing to stderr
. IIRC stderr
is unbuffered on Linux and macOS, but I don't think that's true on Windows.
The original intention might have been to flush both stderr/stdout (all output streams) assuming stderr was unbuffered and thus didn't need flushing.
I've submitted a PR (#10480), so we can continue the conversation over there.
It seems like this should only matter on Windows, could make sense to always flush both regardless of where we're writing to, something like this:
/// Builtin utility to print to stdout/stderr
pub fn op_print(
_state: &mut OpState,
args: (String, bool),
_zero_copy: Option<ZeroCopyBuf>,
) -> Result<(), AnyError> {
let (msg, is_err) = args;
if is_err {
eprint!("{}", msg);
} else {
print!("{}", msg);
}
// Flush all output streams (stderr & stdout)
stdout().flush().unwrap();
stderr().flush().unwrap();
Ok(())
}
Follow up to #10449