-
Notifications
You must be signed in to change notification settings - Fork 73
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
added Linux compatibility #132
base: master
Are you sure you want to change the base?
Conversation
There are a lot of whitespace changes in this. Was this on purpose or is it your editor deciding this? I think it's best if the PR is just the logic changes. We can always go and do whitespace fixes in a dedicated change. |
I suspect it's Atom doing it. ¯_(ツ)_/¯. |
Ah, sorry missed that it was Atom. I'm not comfortable accepting this PR with all the whitespace changes, but it might be ok with @Anviking |
I'll split it into 2 PRs (I think that's an amicable solution). |
&& lhs.path == rhs.path | ||
&& lhs.rootObject as AnyObject === rhs.rootObject as AnyObject | ||
&& lhs.rootObject as? AnyObject === rhs.rootObject as? AnyObject | ||
} | ||
|
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.
Looking at this change, the problem I see with it is if the cast to AnyObject fails for both sides, then the operator will return true
even though they may be different objects.
I'm not sure, but with Swift 3, is it possible to use as Any
instead?
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.
Well, AnyObject
is supposed to represent reference types. Technically, since DecodingError.Metadata
(both lhs
and rhs
is a struct, this should fail 🤔. Can you confirm @Anviking ?
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.
Also, of note, this comment
// FIXME: I'm not sure about === equality
We may be right about this 😅
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.
Yeah, ok, so looking at this, the objects in the meta are Any, so this is trying to convert to a ref type so it can compare pointers (the === operator). I think. This may also be a holdover from the swift 2.x codebase that is no longer necessary.
I'll check to see if we can use === on Any in swift 3.
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.
Ok, confirmed. One cannot use ===
on Any
We're going to have to think up a different bit of logic to make this right.
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 only real problem with the as?
is that it generates a warning: "Any to AnyObject always succeeds"
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 only real problem with the as? is that it generates a warning: "Any to AnyObject always succeeds"
When you think about it, this shouldn't be true. What happens when you cast a struct to AnyObject
?
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.
When you think about it, this shouldn't be true. What happens when you cast a struct to AnyObject ?
On linux, no, on macOS and iOS, yes:
28> struct A {}
29> let a = A()
a: A = {}
error: could not fetch result -- error: Couldn't apply expression side effects : Couldn't dematerialize a: corresponding symbol wasn't found
30> a as AnyObject
$R2: AnyObject = {
instance_type = 0x0000000101001220
}
This looks like a swift language bug/inconsistency to me. If nothing else, we could use #if os
to limit warnings to linux.
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.
Yeah, I think using if OS
and adding an issue on https://github.com/apple/swift should be good enough for now.
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.
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.
@codeOfRobin I would be reluctant to change the whitespace, but yeah, at least not in this PR.
(Also, testing this new review thing for the first time…)
&& lhs.path == rhs.path | ||
&& lhs.rootObject as AnyObject === rhs.rootObject as AnyObject | ||
&& lhs.rootObject as? AnyObject === rhs.rootObject as? AnyObject | ||
} | ||
|
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 to AnyObject
casting was a part of the Swift 3 migration because NSDictionary
accessors now returns Any
, which here make things more complicated.
I'm not aware of a case where an object in an NSDictionary
wouldn't be an AnyObject
. If a struct would end up there though, it would (from what I've seen) get wrapped in some kind of wrapper class (at least on non-linux platforms) and the ===
equality would fail.
So to achieve the same result but with linux compatibility i think it should be enough to guard let as?
unwrap them and return false otherwise.
importing changes
BTW @Anviking There's another reason a linux port would be hard. NSDictionary <-> Swift dictionary conversion on Linux is buggy as hell. For example, let y: [String: Int] = ["a": 1]
guard let x = y as? NSDictionary else {
print("not succeeded")
exit(1)
} will work on MacOS but will fail on Linux ( Turns out, this won't be possible on Linux at all (I posted this bug earlier today): https://bugs.swift.org/browse/SR-3411 Does this mean, there's no way to get Decodable onto Linux? |
@codeOfRobin I think the dependency on |
Though it still will generate warnings on MacOS. Another thing that's weird is,
swift build
won't work because of the files in theTests
folder.Someone who's more well versed with Swift Package manager should probably check up on this(I am open to discussing possible restructuring in the comments).
The changes are, otherwise, super small (adding
?
s. Though Atom seems to have messed up the whitespace somehow ¯\_(ツ)_/¯