Companion PR to Swift #1157 and 32-bit bug fixes #249
Conversation
| @@ -62,7 +62,7 @@ public class NSXMLDTDNode : NSXMLNode { | |||
| } | |||
| } //primitive | |||
|
|
|||
| public override init(kind: NSXMLNodeKind, options: Int) { | |||
| public override init(kind: NSXMLNodeKind, options: NSXMLNodeOptionRawType) { | |||
There was a problem hiding this comment.
This seems like an API change. It is declared in both 32 bit and 64 bit Darwin targets as public convenience init(kind: NSXMLNodeKind, options: Int) why can't it be that here too?
There was a problem hiding this comment.
To avoid changing the API, I can cast it to that type within the function if that's what you prefer. We've hit this problem before in #169 (comment) Ultimately, it seems like this stuff boils down to a CF type that resolves to UInt32. I kinda don't understand why it's promoted to a 64-bit int in Foundation.
There was a problem hiding this comment.
I think because NSUInteger and NSInteger were preferred for APIs for future proofing the storage size for parameters.
There was a problem hiding this comment.
That makes sense. In that case, the type seems necessary. The reason being is that Int on 32-bit platforms is (as expected) a signed 32-bit integer. In NSXMLNodeOptions:105 the upper bits are set to '1' (including bit 31) which is an overflow in that case.
Because NSXMLNodeOptionRawType is a typealias to Int on 64-bit machines and Int64 on 32-bit machines Swift doesn't require casts (to my knowledge) on 64-bit. On 32-bit machines, it guarantees that NSXMLNodePreserveAll won't overflow (among other issues).
There was a problem hiding this comment.
I went ahead and took this stuff out. It's probably not appropriate to mix these into the same PR. I'll make another one for the XML issues.
|
@swift-ci Please test |
|
oh never mind swiftlang/swift#1157 is required to build; else we get |
|
Yep that's right. We'll need to wait a bit. :) Here's the new PR that handles XML #252 |
|
@swift-ci Please test |
|
@phausler It looks like we are going to merge swiftlang/swift#1157, would you mind us merging this PR as well, assuming all tests pass on Linux/x86_64? |
|
I'm a little confused about why the XML change is required for this. |
|
@parkera Oops! It wasn't supposed to be there. The XML stuff was required for Foundation to build on ARM, so I pulled it in prior to Philippe merging it into master. I've removed it. |
|
Ok, this makes more sense to me now. @gribozavr , if you test this and it works you can merge it in. |
|
@parkera Thanks! We're waiting for a companion PR for a similar build system change in xctest, but once that is ready, I'll merge all three PRs together. |
Companion PR to Swift #1157 and 32-bit bug fixes
Add default value to DiagnosticRelatedInformation initializer
This PR is a necessary companion to swiftlang/swift#1157 . swift.ld is used in the build of Foundation, and with the deletion of the script from Swift, Foundation fails to build. The first commit uses the conformance begin and end objects generated in Swift for manual share library linking. Also, this commit allows the Swift build scripts to indicate to Foundation whether it should use the gold linker.
It's very important that this PR only be merged at the same time as the Swift#1157
The second commit addresses some new regressions introduced inNSXML*on 32-bit systems. TheNSXMLNodeOptionRawTypetype was introduced for working withNSXMLNodeOptions in a bit-width independent way.