WIP: bridge BOOL to Bool #24240

compnerd · 2019-04-24T15:12:49Z

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

compnerd · 2019-04-24T15:13:47Z

CC: @DougGregor @jrose-apple - would you guys mind providing early feedback for this? It requires some tests still, but, I think will hugely improve ergonomics on Windows.

jrose-apple

Seems like a good idea to me!

lib/ClangImporter/ImportType.cpp

lib/IRGen/GenClangType.cpp

stdlib/public/core/CompilerProtocols.swift

stdlib/public/Windows/WinSDK.swift

include/swift/AST/KnownIdentifiers.def

lib/AST/ASTContext.cpp

lib/ClangImporter/ImportType.cpp

compnerd · 2019-04-25T00:49:59Z

This is largely ready for review. I want to do a re-test on my Windows machine for the tests and more importantly verify that I don't break the Foundation build with this. The latter would be a follow parallel commit to Foundation.

compnerd · 2019-04-25T18:32:36Z

@swift-ci please test

swift-ci · 2019-04-25T18:34:39Z

Build failed
Swift Test Linux Platform
Git Sha - 589aa12400fc0f201684b0dbe9ea79e70509d9a8

swift-ci · 2019-04-25T19:09:50Z

Build failed
Swift Test OS X Platform
Git Sha - f0ea852f87212ad95b9c0328344e90ab1f4d5de6

compnerd · 2019-04-25T20:33:33Z

@swift-ci please test

xwu

Excited for this improvement. Just a few commenting nits. (Note that publicly facing documentation in the Swift standard library capitalizes “Boolean,” because Boole.)

lib/ClangImporter/ImportDecl.cpp

stdlib/public/core/CompilerProtocols.swift

test/SILGen/win_bool_bridging.swift

compnerd · 2019-04-25T22:33:27Z

Ah, didn't know that it was being capitalised in reference to Boole - not really certain if that is proper or not, but I've changed it anyways. I didn't accept the changes since I was afraid it was going to create individual commits for the accepts.

compnerd · 2019-04-25T22:35:00Z

@swift-ci please test

lib/ClangImporter/ImportDecl.cpp

jrose-apple · 2019-04-25T16:02:15Z

lib/SIL/Bridging.cpp

@@ -140,6 +140,9 @@ Type TypeConverter::getLoweredCBridgedType(AbstractionPattern pattern,
        return t;
      if (builtinTy->getKind() == clang::BuiltinType::UChar)
        return getDarwinBooleanType();
+      if (builtinTy->getKind() == clang::BuiltinType::Int)
+        return getWindowsBOOLType();
+      // TODO(compnerd) do we need to map BOOL?


I'd look and see what we do for DarwinBoolean, unless you want Bool to be exported back to Windows as BOOL in @cdecl functions, in which case you should look for ObjCBool. The way to test this is to try to use WindowsBool in a @cdecl function.

Actually, I do want Bool to be exported back to Windows as BOOL. I will see if I can find that!

Maybe let's do that in a follow-up patch, actually? It's not like we say @_cdecl works anyway.

Yeah, I like that idea.

This allows the conversion of the Windows `BOOL` type to be converted to `Bool` implicitly. The implicit bridging allows for a more ergonomic use of the native Windows APIs in Swift. Due to the ambiguity between the Objective C `BOOL` and the Windows `BOOL`, we must manually map the `BOOL` type to the appropriate type. This required lifting the mapping entry for `ObjCBool` from the mapped types XMACRO definition into the inline definition in the importer. Take the opportunity to simplify the mapping code. Adjust the standard library usage of the `BOOL` type which is now eclipsed by the new `WindowsBool` type, preferring to use `Bool` whenever possible. Thanks to Jordan Rose for the suggestion to do this and a couple of hints along the way.

compnerd · 2019-04-26T02:03:50Z

@swift-ci please test

swift-ci · 2019-04-26T02:05:53Z

Build failed
Swift Test Linux Platform
Git Sha - e0edc2f87702b0f79cc26aee7551880ca558343b

compnerd · 2019-04-26T05:05:22Z

@swift-ci please test macOS platform

swift-ci · 2019-04-26T09:16:12Z

Build failed
Swift Test OS X Platform
Git Sha - 7bf0a75aecec8e6b618d5005304083def462af0b

compnerd · 2019-04-26T14:48:02Z

@swift-ci please test macOS platform

jrose-apple · 2019-04-26T15:56:24Z

test/SILGen/win_bool_bridging.swift

@@ -0,0 +1,30 @@
+// RUN: %target-swift-frontend -typecheck %s -I %clang-importer-sdk-path/usr/include -verify
+// REQUIRES: OS=windows-msvc


Instead of a REQUIRES here, can you make a fake overlay and use an explicit triple? That way we can make sure not to break this from other platforms.

I was trying to do that, but, I could not get enough of Bool to make that work. I end up having to implement Bool, Int8, the operator groups, etc. Because the bridged type is in WinSDK, we need to build a WinSDK module, which requires the Swift module which requires the standard library for Windows to be stubbed out. Is there an easy trick for that?

Hm, right. We probably should have a way to do that at some point, but right now we don't. All right, never mind.

jrose-apple reviewed Apr 24, 2019

View reviewed changes

lib/ClangImporter/ImportType.cpp Outdated Show resolved Hide resolved

lib/IRGen/GenClangType.cpp Outdated Show resolved Hide resolved

stdlib/public/core/CompilerProtocols.swift Outdated Show resolved Hide resolved

stdlib/public/Windows/WinSDK.swift Outdated Show resolved Hide resolved

compnerd force-pushed the bridge-to-terabithia branch from f0ea852 to 935b84c Compare April 24, 2019 20:30

jrose-apple reviewed Apr 24, 2019

View reviewed changes

include/swift/AST/KnownIdentifiers.def Outdated Show resolved Hide resolved

lib/AST/ASTContext.cpp Outdated Show resolved Hide resolved

lib/ClangImporter/ImportType.cpp Outdated Show resolved Hide resolved

compnerd marked this pull request as ready for review April 25, 2019 00:36

compnerd force-pushed the bridge-to-terabithia branch from 935b84c to 589aa12 Compare April 25, 2019 00:48

compnerd force-pushed the bridge-to-terabithia branch from 589aa12 to 7bf0a75 Compare April 25, 2019 17:17

compnerd force-pushed the bridge-to-terabithia branch from 7bf0a75 to e6382e1 Compare April 25, 2019 20:25

xwu reviewed Apr 25, 2019

View reviewed changes

compnerd force-pushed the bridge-to-terabithia branch from e6382e1 to e0edc2f Compare April 25, 2019 22:34

jrose-apple reviewed Apr 25, 2019

View reviewed changes

compnerd force-pushed the bridge-to-terabithia branch from e0edc2f to 83b2904 Compare April 26, 2019 00:52

compnerd mentioned this pull request Apr 26, 2019

Foundation: update for BOOL bridging apple/swift-corelibs-foundation#2177

Merged

jrose-apple reviewed Apr 26, 2019

View reviewed changes

compnerd merged commit f88be05 into apple:master Apr 26, 2019

compnerd deleted the bridge-to-terabithia branch April 26, 2019 18:13

compnerd mentioned this pull request Apr 25, 2019

[SR-9383] bridge BOOL to bool #51849

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: bridge BOOL to Bool #24240

WIP: bridge BOOL to Bool #24240

compnerd commented Apr 24, 2019

compnerd commented Apr 24, 2019

jrose-apple left a comment

compnerd commented Apr 25, 2019

compnerd commented Apr 25, 2019

swift-ci commented Apr 25, 2019

swift-ci commented Apr 25, 2019

compnerd commented Apr 25, 2019

xwu left a comment

compnerd commented Apr 25, 2019

compnerd commented Apr 25, 2019

jrose-apple Apr 25, 2019

compnerd Apr 25, 2019

jrose-apple Apr 26, 2019

compnerd Apr 26, 2019

compnerd commented Apr 26, 2019

swift-ci commented Apr 26, 2019

compnerd commented Apr 26, 2019

swift-ci commented Apr 26, 2019

compnerd commented Apr 26, 2019

jrose-apple Apr 26, 2019

compnerd Apr 26, 2019 •

edited

jrose-apple Apr 26, 2019

		@@ -0,0 +1,30 @@
		// RUN: %target-swift-frontend -typecheck %s -I %clang-importer-sdk-path/usr/include -verify
		// REQUIRES: OS=windows-msvc

WIP: bridge BOOL to Bool #24240

WIP: bridge BOOL to Bool #24240

Conversation

compnerd commented Apr 24, 2019

compnerd commented Apr 24, 2019

jrose-apple left a comment

Choose a reason for hiding this comment

compnerd commented Apr 25, 2019

compnerd commented Apr 25, 2019

swift-ci commented Apr 25, 2019

swift-ci commented Apr 25, 2019

compnerd commented Apr 25, 2019

xwu left a comment

Choose a reason for hiding this comment

compnerd commented Apr 25, 2019

compnerd commented Apr 25, 2019

jrose-apple Apr 25, 2019

Choose a reason for hiding this comment

compnerd Apr 25, 2019

Choose a reason for hiding this comment

jrose-apple Apr 26, 2019

Choose a reason for hiding this comment

compnerd Apr 26, 2019

Choose a reason for hiding this comment

compnerd commented Apr 26, 2019

swift-ci commented Apr 26, 2019

compnerd commented Apr 26, 2019

swift-ci commented Apr 26, 2019

compnerd commented Apr 26, 2019

jrose-apple Apr 26, 2019

Choose a reason for hiding this comment

compnerd Apr 26, 2019 • edited

Choose a reason for hiding this comment

jrose-apple Apr 26, 2019

Choose a reason for hiding this comment

compnerd Apr 26, 2019 •

edited