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

std.fmt.format — with streams #4628

Merged
merged 19 commits into from
Mar 13, 2020
Merged

std.fmt.format — with streams #4628

merged 19 commits into from
Mar 13, 2020

Conversation

fengb
Copy link
Contributor

@fengb fengb commented Mar 4, 2020

Spiritual successor to #4059. Thanks to @daurnimator for suggesting this approach.

Like generators, we can coalesce context/output/error into one argument.

Unlike generators, we don't have to invent a new pattern. OutStream is already an existing standard, and we just have to leverage its functionality. This also circumvents the previously encountered generator footguns since we have at most 1 async context.

The major downside is that OutStream is currently "all or nothing" when it comes to async, which would prevent non-async version of format(). Not sure how much of a problem this is and it's not specific to just format(), and this could be solvable by introducing noasync OutStreams (yay colors?)
solved via #4710

@mikdusan mikdusan added the standard library This issue involves writing Zig code for the standard library. label Mar 6, 2020
@fengb fengb force-pushed the format-stream branch 3 times, most recently from 454fece to 9b8ae23 Compare March 6, 2020 16:01
@ikskuh
Copy link
Contributor

ikskuh commented Mar 7, 2020

The major downside is that OutStream is currently "all or nothing" when it comes to async, which would prevent non-async version of format(). Not sure how much of a problem this is and it's not specific to just format(), and this could be solvable by introducing noasync OutStreams (yay colors?)

I think this can be a major problem when doing user interfaces. I use std.fmt primarily for user interaction and UI formatting. Requiring std.fmt.format to be async because it's using streams would mean that even just setting the text of a label would require to be async. Sure one can use noasync, but i actually like the current approach to format.

@fengb
Copy link
Contributor Author

fengb commented Mar 11, 2020

@MasterQ32 I think the new OutStream rewrite solves the always-async problem, and it exposes that the concept of an OutStream is just Context, Error, writeFn glued together into one.

@@ -155,7 +151,7 @@ pub const Buffer = struct {
}

pub fn print(self: *Buffer, comptime fmt: []const u8, args: var) !void {
return std.fmt.format(self, error{OutOfMemory}, Buffer.append, fmt, args);
return self.outStream().print(fmt, args);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that the stream is readily available, maybe this method should be deleted.

@fengb fengb marked this pull request as ready for review March 12, 2020 16:29
@andrewrk
Copy link
Member

This idea is approved ✔️ but I haven't reviewed the PR yet

@andrewrk andrewrk merged commit 2dd920e into ziglang:master Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants