-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Integers in Swift were down casted to 32 bits integers. Fixed #688 #695
Conversation
@@ -86,7 +86,7 @@ import Foundation | |||
case is Bool.Type, is Bool?.Type: | |||
(p, t) = (RLMProperty(name: name, type: .Bool, objectClassName: nil, attributes: attr), "c") | |||
case is Int.Type, is Int?.Type: | |||
(p, t) = (RLMProperty(name: name, type: .Int, objectClassName: nil, attributes: attr), "i") | |||
(p, t) = (RLMProperty(name: name, type: .Int, objectClassName: nil, attributes: attr), "l") |
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.
Can we check for __LP64__
here too and use "i" if not defined?
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 reason I didn't do it is because that conditional check doesn't work (just yet?) on swift. Actually a fun fact is: if you explore the internal attributes (using objc_getattributes) of the Int type from swift (either 32bits or 64bits). The result is always "l".
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.
By "doesn't work" I mean LP64 is not defined. I didn't dig on why clang is not including it tho.
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.
Can we add a FIXME comment so we know fix this later? I think in the long run we will have to move this code to objc to make this work, but for now this is an improvement.
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 can add a FIXME, but I think the entire swift file should be replaced eventually. Maintaining the reflection logic in two places is going to be a PIA on the long run. I'm hopping that the next beta would come with a way from objc to get a Swift property type (main problem right now is String which is reflected as "@" from objc). Hopefully it'd be a way of doing this in the near future and this files (rather than this particular line) can be replaced.
And to be honest "l" is the safest choice anyway. If we could do a LP64 conditional, Swift Int type on 32bits would be "l" and on 64bits would be "q"
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.
Instead of checking for __LP64__
, we could do the following:
p = RLMProperty(name: name, type: .Int, objectClassName: nil, attributes: attr)
#if arch(x86_64) || arch(arm64)
t = "l"
#else
t = "i"
#endif
Thoughts @jpsim ? |
Thanks for your work on this, @Reflejo, it's certainly an improvement. I'd be happy merging this in, if we do the platform check I mentioned ( |
Preprocessor statement added. |
This works for me, @alazier any other remarks? |
Integers in Swift were down casted to 32 bits integers. Fixed #688
let it merge |
When using Swift, Int type is Int32 on 32bits platforms and Int64 on 64 bits platforms. In order to avoid down casting, objcType should be "l" when using 64 bits.
Also added some tests. Note that when assigning this on 32bits platforms:
long values will be down casted to 32 bits. Meaning that 17179869184 is going to be 0 as well as 8589934592. But since the value inserted will be 0 the test will pass.
I recommend supporting Int64 as well as Int32 explicitly on Realm.
See #688 for more information.