Skip to content

Use SWIFT_SDK_OVERLAY_DISPATCH_EPOCH to detect that an overlay is bei… #97

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

dgrove-oss
Copy link
Contributor

@dgrove-oss dgrove-oss commented Jul 5, 2016

…ng built

Instead of defining the special macro DISPATCH_BUILDING_SWIFT_MODULE
to detect that dispatch headers are being processed by the clang importer,
check to see if SWIFT_SDK_OVERLAY_DISPATCH_EPOCH is defined.

The prime motivation for doing this is to avoid needing to define DISPATCH_BUILDING_SWIFT_MODULE when compiling a Swift program that imports Dispatch. A secondary benefit is that it makes additional functions unavailable for Swift3 (eg dispatch_once).

This depends on a pull request (swiftlang/swift#3346) to Swift to change the importer to define SWIFT_SDK_OVERLAY_DISPATCH_EPOCH on non-Darwin platforms.

@MadCoder
Copy link
Contributor

MadCoder commented Jul 5, 2016

Seems reasonable but I'd like for @mwwa to double check.

@jrose-apple
Copy link
Contributor

This doesn't seem correct to me. Won't it be set whenever any Swift code is used, not just when building the overlay?

@dgrove-oss
Copy link
Contributor Author

The problem I'm trying to work around could be a symptom of me not teaching the makefiles how to build the Dispatch and CDispatch modules correctly. I've tried a bunch of things, but I could have missed something.

The problem is that the current approach of defining DISPATCH_BUILDING_SWIFT_MODULE only when actually building the overlay doesn't work very well. When a user imports Dispatch into their Swift program, they also need to define DISPATCH_BUILDING_SWIFT_MODULE or the import of the dispatch header files fails. It appears that when the Dispatch module is imported by a user program, the Swift compiler goes off, looks at the modulemap file that defines the Dispatch and CDispatch modules, and attempts to reimport the C dispatch header files for CDispatch. This import fails because DISPATCH_BUILDING_SWIFT_MODULE isn't defined (unless the user magically knows to do that via a command line argument to swiftc).

If there were a way to build the swiftmodule that would cut off this import when users import the module, that would be great and we could do that instead.

A related thing that would be great to fix, when the user imports Dispatch, they currently need to add -Xcc -fblocks to the swiftc command line manually. I'd really like to make that automatic (triggered by importing Dispatch), but don't know a good way of doing that.

@jrose-apple
Copy link
Contributor

So DISPATCH_BUILDING_SWIFT_MODULE really just means "Dispatch is going to be used from Swift"? :-/ We've mostly been avoiding providing a general query of that kind—so that people don't accidentally break the Swift side of that branch, or accidentally not expose some API—but maybe we need to give up on that. (I wouldn't want to make it Dispatch-specific, though.)

On the related thing: we're already considering just turning on blocks all the time for Swift-on-Linux, but we're wondering if we need to make them AnyObject-compatible first. I'm not in most of those discussions, though.

@MadCoder
Copy link
Contributor

MadCoder commented Jul 5, 2016

@jrose-apple there's no way you can have something like dispatch work with swift without an equivalent of #ifdef __OBJC__ but for swift because it's predating code that is made swifty, and not a new Swift API that has a C backend.

IOW I argue that the swift importer should have a #ifdef __SWIFT__ and that should be valued to the language version like C and C++ do.

@jrose-apple
Copy link
Contributor

The original idea was that swift-corelibs-dispatch and any other framework not shipped with the OS should always be Swifty, at least on non-Apple platforms. In particular, since you're only using this guard to protect against the code being considered non-modular, checking for __has_feature(modules) is probably a better idea anyway, because that helps C clients too.

@mwwa
Copy link
Contributor

mwwa commented Jul 5, 2016

(As an aside: The Darwin version of this check uses it to alter the class hierarchy appearance under Swift, something that really does need to be checking for Swift-and-not-C, so that we don't alter the API that we've historically presented to Obj-C clients)

@jrose-apple
Copy link
Contributor

Yeah, we probably will need this at some point. I just want to do it properly when we get there, including discussing it on swift-evolution.

@MadCoder
Copy link
Contributor

MadCoder commented Jul 5, 2016

@jrose-apple the problem with this is that the codebase is shared with macOS where it is not the case, and I don't think we should break people using libdispatch as a C library outside of swift on Linux. It actually is somehow broken today because objects are wrapped, but for the sake of interoperating with say, CF level APIs, being able to address libdispatch in C is valuable even on non macOS platforms.

so I don't think the intent works in practice, however pure and noble it is :)

Just for the sake of porting and testing libdispatch on new platforms it's much easier if you can isolate dispatch from Swift.

@MadCoder
Copy link
Contributor

MadCoder commented Jul 5, 2016

also __has_feature(modules) is not what we want because it would inject <stdio.h> on Darwin platforms where you build in C with modules.

@dgrove-oss
Copy link
Contributor Author

dgrove-oss commented Jul 5, 2016

fwiw, I think having a SWIFT defined by the importer would be quite reasonable. In the very short term, the has_feature(modules) does work (as one would expect) and is inside a __linux. So we would take that as a an interim fix if you don't want to define the epoch off Darwin and getting SWIFT would take a full swift evolution.

I'd like to get the Swift3 overlay workable as soon as possible so we can start using it for corelibs-foundation.

@MadCoder
Copy link
Contributor

MadCoder commented Jul 5, 2016

Right since it's inside a __linux__ guard then let's go with the modules feature test please @dgrove-oss

@jrose-apple
Copy link
Contributor

I'll bring up __SWIFT__ on swift-dev, and we can move it to swift-evolution once we have a direction.

Instead of using a special macro, check for __has_feature(modules) to
activate including stdio.h in dispatch.h on Linux.

Also, fix one place where OS_OBJECT_SWIFT_3 was used outside of an
OS_OBJECT_USE_OBJC block (to prepare for SWIFT_SDK_OVERLAY_DISPATCH_EPOCH
being defined by the clang importer on non-Darwin platforms).
@dgrove-oss dgrove-oss force-pushed the define-dispatch-bridging-macro-on-linux branch from 8c5a6af to 0f402b8 Compare July 6, 2016 13:58
@dgrove-oss
Copy link
Contributor Author

updated to modules features test & squashed to single commit.

@MadCoder MadCoder merged commit 92689ed into swiftlang:master Jul 6, 2016
@dgrove-oss dgrove-oss deleted the define-dispatch-bridging-macro-on-linux branch July 6, 2016 14:40
das pushed a commit that referenced this pull request Aug 11, 2016
…-on-linux

Use SWIFT_SDK_OVERLAY_DISPATCH_EPOCH to detect that an overlay is bei…

Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>
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.

4 participants