-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Add "automatic" JSON facilities #4304
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
|
Vectors? |
|
I don't understand the CI failure This line is directly following a check of the tag type: testing.expectEqual(@TagType(T).string, @as(@TagType(T), r));
testing.expectEqualSlices(u8, "withąunicode", r.string); |
|
Might be #4295? No idea why the previous line works. |
4cbb652 to
14fcc92
Compare
|
Bah; rebased on master and CI is still failing on that one line of code. The failure seems to only happen on aarch64. |
|
Bad news: you're hitting a LLVM bug! Consider this small piece of LLVM IR: ; ModuleID = 'foo'
source_filename = "foo"
target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
target triple = "aarch64v8.1a-unknown-linux-musl"
define internal fastcc void @foobar(i2*) unnamed_addr {
Entry:
store i2 -2, i2* %0
ret void
}LLVM 9 assembles it as: mov w8, #254
strb w8, [x0]
retNotice the wrong integer literal 😱 every other backend gets it right so nobody hit this bug before: movb $2, (%rdi)
retqThe good news is that LLVM 10 already fixed this problem and produces the following asm: mov w8, #2
strb w8, [x0]
ret |
Not really, we'd have to widen the enum base type to a minumum of 8 bits... but that'd wreak havoc on pretty much every other piece of code :\ |
85ed17a to
b78b382
Compare
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.
This is some really nice work, and a useful abstraction as well.
This is very close to merge-ready. I'm here to help get it landed; reach out if you have any questions or want to hand it off.
| } | ||
|
|
||
| pub const JsonDumpOptions = struct { | ||
| // TODO: indentation options? |
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.
you could take inspiration from
zig/lib/std/json/write_stream.zig
Lines 24 to 31 in e8a8492
| /// The string used for indenting. | |
| one_indent: []const u8 = " ", | |
| /// The string used as a newline character. | |
| newline: []const u8 = "\n", | |
| /// The string used as spacing. | |
| space: []const u8 = " ", |
| pub const JsonDumpOptions = struct { | ||
| // TODO: indentation options? | ||
| // TODO: make escaping '/' in strings optional? | ||
| // TODO: allow picking if []u8 is string or array? |
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.
This seems like it should be somehow configurable on a per-field basis. Not necessary to solve this problem for this PR though.
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 looked into adding support for overriding on a per-type basis:
pub const Override = struct {
comptime T: type,
comptime jsonStringify: fn (
value: T,
options: StringifyOptions,
context: var,
comptime Errors: type,
comptime output: fn (@TypeOf(context), []const u8) Errors!void,
) Errors!void,
};
comptime overrides: []Override = &[_]Override,However this hit a few issues:
- The "comptime struct field missing initialization value" error codepath doesn't seem to consider default values
- I am unable to refer to the T field from the function type declaration: "error: use of undeclared identifier 'T'"
- I get
error: use of undeclared identifier 'context'for that function type declaration
| // TODO: using switches here segfault the compiler (#2727?) | ||
| if ((stringToken.escapes == .None and mem.eql(u8, field.name, key_source_slice)) or (stringToken.escapes == .Some and (field.name.len == stringToken.decodedLength() and encodesTo(field.name, key_source_slice)))) { | ||
| // if (switch (stringToken.escapes) { | ||
| // .None => mem.eql(u8, field.name, key_source_slice), | ||
| // .Some => (field.name.len == stringToken.decodedLength() and encodesTo(field.name, key_source_slice)), | ||
| // }) { | ||
| if (fields_seen[i]) { | ||
| // switch (options.onDuplicateField) { | ||
| // .UseFirst => {}, | ||
| // .Error => {}, | ||
| // .UseLast => {}, | ||
| // } |
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.
this is so ugly, that I'm motivated to try to solve #2727
(not a merge blocker though)
b78b382 to
e270db9
Compare
Recreating #3155 (github doesn't seem to allow reopening PRs)