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

Define mappings for CF types on LLP64 #20717

Merged
merged 1 commit into from Nov 28, 2018
Merged

Conversation

troughton
Copy link
Contributor

@troughton troughton commented Nov 25, 2018

As per https://forums.swift.org/t/llp64-targets-and-integral-types/18253/15, define mappings for CFTypeID, CFOptionFlags, and CFHashCode to UInt.

The DefineAndUse parameter may be incorrect; it wasn't clear to me what its value should be.

As an alternative solution, CoreFoundation could be updated to typedef these types to e.g. uintptr_t, which is already mapped to UInt on LLP64.

cc @jckarter and @compnerd

@xwu
Copy link
Collaborator

xwu commented Nov 25, 2018

@swift-ci Please smoke test

@jckarter
Copy link
Member

Should also cc @phausler and @millenomi. Looks good to me.

@compnerd
Copy link
Collaborator

@jrose-apple probably makes sense too

@jrose-apple
Copy link
Contributor

Needs tests. I'm worried about picking UInt over Int for these, too, because that's source-breaking. I think we'd have better luck keeping CFTypeID and CFHashCode as Int; we may be able to get away with CFOptionFlags, but it's still a risk.

@swift-ci Please test source compatibility

@jrose-apple
Copy link
Contributor

The DefineAndUse parameter may be incorrect; it wasn't clear to me what its value should be.

I think this is reasonable. This means that the name "CFHashCode" remains available in Swift, and that places where that type is currently used in C APIs should continue to do so instead of using the underlying type.

Also, thank you for looking into this!

@phausler
Copy link
Member

what impact does this have on Darwin? does it have any effect on linux? I dont see anything that limits this to just LLP64 (maybe I am missing something?)

@troughton
Copy link
Contributor Author

I'm worried about picking UInt over Int for these, too, because that's source-breaking. I think we'd have better luck keeping CFTypeID and CFHashCode as Int; we may be able to get away with CFOptionFlags, but it's still a risk.

On macOS at least (and I assume everywhere other than LLP64) these types are already imported as UInt.

There shouldn't be any effect on any platform other than Windows x64; this just makes it explicit so these types are imported as UInt rather than UInt64 there, consistent with other platforms.

@jrose-apple
Copy link
Contributor

On macOS at least (and I assume everywhere other than LLP64) these types are already imported as UInt.

I feel silly now. These aren't defined in terms of CFIndex or (obviously) NSUInteger. Yes, this looks good to me, although it does need a regression test. Copying the declarations of the types into the mock SDK's CoreFoundation.h and then checking that the types are compatible in one of the test/ClangImporter/ files would be sufficient.

As per https://forums.swift.org/t/llp64-targets-and-integral-types/18253/15, define mappings for `CFTypeID`, `CFOptionFlags`, `CFHashCode`  to `UInt` for LLP64 support.
@troughton
Copy link
Contributor Author

Tests have passed locally, so this should be good to test and merge now.

@jrose-apple
Copy link
Contributor

@swift-ci Please test

@jrose-apple
Copy link
Contributor

Source compat tests, in case GitHub decides to hide them: Debug, Release

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - bd2e73463b6bd19667a0623f3309bd641df6ae8f

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - bd2e73463b6bd19667a0623f3309bd641df6ae8f

@jrose-apple
Copy link
Contributor

jrose-apple commented Nov 27, 2018

I marked this as approved but I'd like to have either Philippe or Lily sign off on it as well. As noted, this should have no change on Darwin or Linux because these typedefs already had an underlying type of Int/UInt; it's only LLP64 systems where the default type wasn't working. (Even LP32 Windows is the same both before and after this change, though I don't know if that's part of Saleem's master plan.)

@phausler
Copy link
Member

The change seems sane to me and I sign off if there is no Darwin impact and we gain Windows conformance to the expected types.

@compnerd
Copy link
Collaborator

Thanks for fixing this @troughton! Seems that @phausler and @jrose-apple have approved the change, going to merge this.

@compnerd compnerd merged commit e4efac5 into apple:master Nov 28, 2018
@tkremenek
Copy link
Member

@compnerd: please also prepare a PR against swift-5.0-branch

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

8 participants