Skip to content

WIP: bridge BOOL to Bool #24240

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 26, 2019
Merged

WIP: bridge BOOL to Bool #24240

merged 1 commit into from
Apr 26, 2019

Conversation

compnerd
Copy link
Member

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

Resolves SR-NNNN.

@compnerd
Copy link
Member Author

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
jrose-apple reviewed Apr 24, 2019
View reviewed changes
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good idea to me!

@compnerd compnerd force-pushed the bridge-to-terabithia branch from f0ea852 to 935b84c Compare April 24, 2019 20:30
jrose-apple
jrose-apple reviewed Apr 24, 2019
View reviewed changes
@compnerd compnerd marked this pull request as ready for review April 25, 2019 00:36
@compnerd compnerd force-pushed the bridge-to-terabithia branch from 935b84c to 589aa12 Compare April 25, 2019 00:48
@compnerd
Copy link
Member Author

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 compnerd force-pushed the bridge-to-terabithia branch from 589aa12 to 7bf0a75 Compare April 25, 2019 17:17
@compnerd
Copy link
Member Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 589aa12400fc0f201684b0dbe9ea79e70509d9a8

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - f0ea852f87212ad95b9c0328344e90ab1f4d5de6

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

@swift-ci please test

xwu
xwu reviewed Apr 25, 2019
View reviewed changes
Copy link
Collaborator

@xwu xwu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@compnerd
Copy link
Member Author

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 compnerd force-pushed the bridge-to-terabithia branch from e6382e1 to e0edc2f Compare April 25, 2019 22:34
@compnerd
Copy link
Member Author

@swift-ci please test

jrose-apple
jrose-apple reviewed Apr 25, 2019
View reviewed changes
lib/SIL/Bridging.cpp Outdated
@@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I like that idea.

@compnerd
Windows: bridge BOOL to Bool
83b2904 
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 compnerd force-pushed the bridge-to-terabithia branch from e0edc2f to 83b2904 Compare April 26, 2019 00:52
@compnerd
Copy link
Member Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - e0edc2f87702b0f79cc26aee7551880ca558343b

@compnerd compnerd mentioned this pull request Apr 26, 2019
Merged
@compnerd
Copy link
Member Author

@swift-ci please test macOS platform

@swift-ci
Copy link
Contributor

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

@compnerd
Copy link
Member Author

@swift-ci please test macOS platform

jrose-apple
jrose-apple reviewed Apr 26, 2019
View reviewed changes
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@compnerd compnerd Apr 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@compnerd compnerd merged commit f88be05 into swiftlang:master Apr 26, 2019
@compnerd compnerd deleted the bridge-to-terabithia branch April 26, 2019 18:13
@compnerd compnerd mentioned this pull request Apr 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xwu xwu xwu left review comments

@jrose-apple jrose-apple jrose-apple left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@compnerd @swift-ci @xwu @jrose-apple