-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Add 'u' specifier to std.format #3970
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
Conversation
| output: fn (@TypeOf(context), []const u8) Errors!void, | ||
| ) Errors!void { | ||
| var buf: [4]u8 = undefined; | ||
| const len = std.unicode.utf8Encode(c, buf[0..]) catch unreachable; |
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.
why is this catch unreachable? what if someone e.g. passed in 0x1fffff
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.
attempt to unwrap error: CodepointTooLarge
Personally I'd prefer to have utf8EncodeUnsafe with 0xFFFD replacement.
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'm OK with this function asserting that the codepoint fits, with a bit more defensive coding:
- Document the assertion in doc comments of the function
- Rather than
catch unreachable,catch |err| switch (err)and then list the error(s) explicitly that are unreachable. This will cause compile errors if new errors are added to the set.
| @compileError("Cannot print integer that is larger than 8 bits as a ascii"); | ||
| } | ||
| } else if (comptime std.mem.eql(u8, fmt, "u")) { | ||
| if (@TypeOf(int_value).bit_count <= 21) { |
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.
Does a comptime int pass this check?
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.
Copy-pasted from c modifier.
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 do want this to handle comptime_int please. The code for the c modifier is also incorrect if it cannot handle them. Until recently, due to limitations of the zig language, comptime_ints could not be passed to formatted printing. However that is now fixed, and comptime ints can be passed.
I would be OK with a non-explicit @compileError call here, allowing the @as to do the job. That would be my personal strategy to handle comptime_int correctly. Note also that the @as is redundant. Every parameter always gets type coerced to the parameter type.
andrewrk
left a comment
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.
The API and doc changes are good 👍
a couple improvements in the implementation and then this is merge-ready.
| @compileError("Cannot print integer that is larger than 8 bits as a ascii"); | ||
| } | ||
| } else if (comptime std.mem.eql(u8, fmt, "u")) { | ||
| if (@TypeOf(int_value).bit_count <= 21) { |
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 do want this to handle comptime_int please. The code for the c modifier is also incorrect if it cannot handle them. Until recently, due to limitations of the zig language, comptime_ints could not be passed to formatted printing. However that is now fixed, and comptime ints can be passed.
I would be OK with a non-explicit @compileError call here, allowing the @as to do the job. That would be my personal strategy to handle comptime_int correctly. Note also that the @as is redundant. Every parameter always gets type coerced to the parameter type.
| output: fn (@TypeOf(context), []const u8) Errors!void, | ||
| ) Errors!void { | ||
| var buf: [4]u8 = undefined; | ||
| const len = std.unicode.utf8Encode(c, buf[0..]) catch unreachable; |
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'm OK with this function asserting that the codepoint fits, with a bit more defensive coding:
- Document the assertion in doc comments of the function
- Rather than
catch unreachable,catch |err| switch (err)and then list the error(s) explicitly that are unreachable. This will cause compile errors if new errors are added to the set.
|
Works for me as is. You got an idea. |
Outputs integer as an UTF-8 sequence.