-
Notifications
You must be signed in to change notification settings - Fork 9
Add OpaquePointer implementation
#34
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
.github/workflows/build.yml
Outdated
|
|
||
| - branch: development | ||
| tag: DEVELOPMENT-SNAPSHOT-2021-09-18-a | ||
| tag: DEVELOPMENT-SNAPSHOT-2022-02-03-a |
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.
These changes are unrelated to the addition of the new type.
| public struct Int { | ||
| @usableFromInline | ||
| internal var _value: Builtin.Int32 | ||
| internal var _value: Builtin.Word |
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 unrelated to the addition of OpaquePointer.
Sources/Core/OpaquePointer.swift
Outdated
|
|
||
| @_transparent | ||
| public init?(bitPattern: Int) { | ||
| guard bitPattern != 0 else { return nil } |
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.
Is there a reason to use guard here rather than if bitPattern == 0 { return nil }?
| public struct UInt { | ||
| @usableFromInline | ||
| internal var _value: Builtin.Int32 | ||
| internal var _value: Builtin.Word |
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.
Unrelated to the addition of OpaquePointer.
Sources/Core/OpaquePointer.swift
Outdated
| @_transparent | ||
| public init?(bitPattern: Int) { | ||
| if bitPattern == 0 { return nil } | ||
| _rawValue = Builtin.inttoptr_Int32(bitPattern._value) |
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 will truncate no?
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.
It will, but I had to use Int32 here, as you've requested Word changes to UInt and Int to be removed from this PR in your comment above.
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 trying to get you to be more methodical about the changes. Clearly there is a bug in Int and UInt, that should be fixed first. Once that is fixed, adding an implementation here is simple.
|
|
||
| - branch: swift-5.5-release | ||
| tag: 5.5-RELEASE | ||
|
|
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.
IMO this is unrelated to implementing OpaquePointer.
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.
These releases crash when attempting to build OpaquePointer code, as can be seen in #31. What would be the best way to make CI green on this PR without removing these releases from the build matrix?
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 can use #if swift(>=5.6) around the implementation of OpaquePointer :)
| // All Rights Reserved. | ||
| // SPDX-License-Identifier: BSD-3 | ||
|
|
||
| #if swift(>=5.6) |
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.
Newlines after this would've been nice, but eh.
| // All Rights Reserved. | ||
| // SPDX-License-Identifier: BSD-3 | ||
|
|
||
| // This `OpaquePointer` implementation is known to crash 5.4 and 5.5 compiler releases on Windows. |
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.
Thanks for the comment, that is a good idea to record!
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 formatter I use has done a stupid (in my opinion) thing by indenting everything within #if block. I've pushed one more time to fix that.
This type is required for a basic
StaticStringimplementation.