Skip to content

ClangImporter: Preserve CChar type #32692

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
Jul 8, 2020
Merged

ClangImporter: Preserve CChar type #32692

merged 1 commit into from
Jul 8, 2020

Conversation

@dduan
Copy link
Contributor

@dduan dduan commented Jul 2, 2020 •
edited
Loading

Preserve CChar and CWideChar when they are imported as part of C signatures.

Resolves SR-466.

@dduan
Copy link
Contributor Author

dduan commented Jul 2, 2020

@swift-ci test

@swift-ci
Copy link
Contributor

swift-ci commented Jul 2, 2020

Build failed
Swift Test OS X Platform
Git Sha - 3fdcd4b8ae68eebfb6eb237a70bb5720b44ed5ec

@CodaFi
Copy link
Contributor

CodaFi commented Jul 2, 2020

Looks good. My one ask is for additional ClangImporter tests for the other char types.

@benrimmington
Copy link
Contributor

benrimmington commented Jul 3, 2020

@compnerd Does the CWideChar typealias in CTypes.swift need to be updated for Windows?

@dduan dduan force-pushed the sr-466 branch 2 times, most recently from 9b344e8 to bb6ebb8 Compare July 3, 2020 23:18
@dduan
Copy link
Contributor Author

dduan commented Jul 3, 2020

@swift-ci test

@swiftlang swiftlang deleted a comment from swift-ci Jul 4, 2020
@swiftlang swiftlang deleted a comment from swift-ci Jul 4, 2020
@swiftlang swiftlang deleted a comment from swift-ci Jul 4, 2020
@swiftlang swiftlang deleted a comment from swift-ci Jul 4, 2020
@dduan
Copy link
Contributor Author

dduan commented Jul 4, 2020 •
edited
Loading

There might be an expectation problem with regard to wide characters. Regardless of this patch, wchar_t gets imported as an integer (Swift.Int32 on macOS). CWideChar is aliased to Unicode.Scalar in CTypes.swift, which is a struct that holds an UInt32 aka an unrelated type. So users expecting to use CWideChar with the imported API are in for a surprise. Same applies for CChar32.

I think similar problem exists on platforms where char is unsigned, in which case even CChar won't work as expected as it's aliased to signed integer.

@compnerd
Copy link
Member

compnerd commented Jul 6, 2020 •
edited
Loading

@benrimmington - yeah, it does, CWideChar should be Swift.Unicode.UTF16.CodeUnit

@dduan - Android and Linux IIRC are unsigned char for char (on ARM/AArch64) and Windows and macOS are signed char (on ARM/ARM64). On x86/x86_64, all of them should be signed char. On PPC/PPC64 there is a bug I believe that only Darwin will be treated as signed char when it should mirror the ARM/AArch64 case.

The more interesting problem here is that we might have to change the mapping based upon -fsigned-char, -fno-signed-char, -fshort-wchar, -fno-short-wchar being passed to clang-importer.

@gribozavr
Copy link
Contributor

gribozavr commented Jul 6, 2020

@dduan Thanks for the fix! Could you add a test to the test/Interop/C directory for both regular and wide chars?

@gribozavr gribozavr added the c++ interop Feature: Interoperability with C++ label Jul 6, 2020
@dduan
Copy link
Contributor Author

dduan commented Jul 7, 2020 •
edited
Loading

@gribozavr Added in a separate commit. Will squash if it looks good to you.

@dduan
Copy link
Contributor Author

dduan commented Jul 7, 2020

@swift-ci test

@swiftlang swiftlang deleted a comment from swift-ci Jul 7, 2020
@swiftlang swiftlang deleted a comment from swift-ci Jul 7, 2020
@belkadan
Copy link
Contributor

belkadan commented Jul 7, 2020

Ah, right, wchar_t is a typedef in C and a built-in type in C++, so we've just been silently doing the wrong thing for a long time because it's not in MappedTypes. :-/

@dduan
Copy link
Contributor Author

dduan commented Jul 8, 2020

@swift-ci please smoke test

@dduan
Copy link
Contributor Author

dduan commented Jul 8, 2020

I've captured discussions here in a few tickets. Thanks to all participants!

https://bugs.swift.org/browse/SR-13174
https://bugs.swift.org/browse/SR-13175
https://bugs.swift.org/browse/SR-13176

@dduan dduan merged commit 942893d into swiftlang:master Jul 8, 2020
@dduan dduan mentioned this pull request Jul 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c++ interop Feature: Interoperability with C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@dduan @swift-ci @CodaFi @benrimmington @compnerd @gribozavr @belkadan