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

JSON Encoding based on JSONValue #2987

Merged
merged 1 commit into from
Mar 4, 2021
Merged

Conversation

fabianfett
Copy link
Member

After #2966 and #2985, this is the last step to make JSON Encoding and Decoding much more performant on Linux.

@drexin
Copy link
Member

drexin commented Mar 3, 2021

@swift-ci test

@inline(__always) func setObject(for key: String) -> Object {
switch self.dict[key] {
case .encoder:
preconditionFailure("For key \"\(key)\" an encoder has already been created.")
Copy link
Member

@tomerd tomerd Mar 3, 2021

Choose a reason for hiding this comment

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

is it safe to precondition here (and other spot around this)? i.e no user action can cause this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This shall lead to a crash because of a programmer error and closely mimics the Darwin Foundation behavior. It is not possible to create a json array or a json dictionary at the same time for the same key.

if #available(macOS 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *) {
return .string(_iso8601Formatter.string(from: date))
} else {
fatalError("ISO8601DateFormatter is unavailable on this platform.")
Copy link
Member

@tomerd tomerd Mar 3, 2021

Choose a reason for hiding this comment

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

hmmm I think we can use regular formatter with right pattern to achieve iso8601, no?

Copy link
Member

Choose a reason for hiding this comment

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

I see it now

}
bytes.append(._closebracket)
case .object(let dict):
if #available(OSX 10.13, *), options.contains(.sortedKeys) {
Copy link
Member

@tomerd tomerd Mar 3, 2021

Choose a reason for hiding this comment

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

ditto, should be supported on linux afaik

Copy link
Member

Choose a reason for hiding this comment

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

I see it now

case nestedArray(Array)
case nestedObject(Object)

class Array {
Copy link
Member

Choose a reason for hiding this comment

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

Could we name this something more specific? I get it's a private type but that case nestedArray(Array) threw me for a second.

@spevans
Copy link
Collaborator

spevans commented Mar 4, 2021

@swift-ci test

Copy link
Member

@bendjones bendjones left a comment

Choose a reason for hiding this comment

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

Gave it another read through LGTM

@bendjones
Copy link
Member

cc @millenomi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants