[cxx-interop] Implement foreign reference types. by zoecarver · Pull Request #39605 · swiftlang/swift

zoecarver · 2021-10-06T01:18:30Z

This is an experimental feature to allow an attribute, import_as_ref, to import a C++ record as a non-reference-counted reference type in Swift.

test/Interop/Cxx/foreign-reference/Inputs/pod.h

rjmccall · 2021-10-06T18:56:46Z

lib/AST/Decl.cpp

  if (auto *classDecl = dyn_cast<ClassDecl>(storage->getDeclContext())) {
-    if (storage->isFinal() || classDecl->isFinal())
+    if (storage->isFinal() || classDecl->isFinal() ||
+        classDecl->isForeignReferenceType())


Why is this here? Imported classes can have polymorphic operations. At the very least, this needs a comment.

Added a comment.

lib/AST/Type.cpp

lib/ClangImporter/ImportDecl.cpp

lib/IRGen/GenCall.cpp

lib/SIL/IR/SILFunctionType.cpp

zoecarver · 2021-10-13T17:42:37Z

@rjmccall this should be ready for re-review now. I think I addressed all your comments. Let me know what you think.

Note: I still need to fix the issue with optionals (unless some macro tells us otherwise, we should be importing foreign reference types as optionals). I'll do that shortly, but I think the rest of the PR can still be reviewed without that change.

lib/SIL/Verifier/SILVerifier.cpp

zoecarver · 2021-10-22T00:13:55Z

@swift-ci please smoke test.

zoecarver · 2021-10-22T00:14:17Z

@rjmccall friendly ping :)

zoecarver · 2021-10-25T21:20:34Z

@swift-ci please smoke test.

zoecarver · 2021-10-28T17:34:33Z

@rjmccall friendly ping :)

zoecarver · 2021-11-20T00:17:50Z

include/swift/AST/Decl.h

This is not ideal, but I think it's OK because we never construct a const decl, right?

zoecarver · 2021-11-20T00:48:32Z

@swift-ci please smoke test.

zoecarver · 2021-11-29T20:58:49Z

@swift-ci please smoke test.

zoecarver · 2021-11-29T21:11:54Z

@rjmccall friendly ping :)

zoecarver · 2021-12-05T19:02:27Z

@swift-ci please smoke test.

zoecarver · 2021-12-05T19:18:14Z

@swift-ci please smoke test.

zoecarver · 2021-12-05T19:18:23Z

@swift-ci please smoke test.

zoecarver · 2021-12-06T08:21:49Z

@swift-ci please smoke test.

zoecarver · 2021-12-06T16:32:40Z

@swift-ci please smoke test Linux.

zoecarver · 2021-12-06T22:26:43Z

@swift-ci please smoke test Linux.

zoecarver · 2021-12-06T22:28:55Z

@swift-ci please smoke test Linux.

zoecarver · 2021-12-08T10:18:06Z

@swift-ci please smoke test Linux.

zoecarver · 2021-12-08T12:01:11Z

@swift-ci please smoke test.

This is an expiremental feature to allow an attribute, `import_as_ref`, to import a C++ record as a non-reference-counted reference type in Swift.

zoecarver · 2021-12-08T15:35:34Z

@swift-ci please smoke test.

hyp · 2021-12-08T18:22:21Z

lib/AST/Type.cpp

+}
+
+bool CanType::isForeignReferenceType() {
+  if (auto *classDecl = getPointer()->lookThroughAllOptionalTypes()->getClassOrBoundGenericClass())


Looks like you duplicate the logic here. Can you call getPointer()->isForeignReferenceType() instead?

zoecarver · 2021-12-08T19:26:58Z

test/Interop/Cxx/foreign-reference/not-any-object.swift

+public func test(_ _: AnyObject) {}
+
+// TODO: make this a better error.
+test(Empty.create()) // expected-error {{type of expression is ambiguous without more context}}


Add a test for casting explicitly to any object.

zoecarver · 2021-12-08T19:29:07Z

test/Interop/Cxx/foreign-reference/Inputs/pod.h

+IntPair passThroughByValue(IntPair x) { return x; }
+
+struct __attribute__((swift_attr("import_as_ref"))) RefHoldingPair {
+  // REVIEW-NOTE: I added support for this but then removed it, as this sort of


Resolve this.

@NoEscape

- swiftlang#58975 switched many tests from XFAIL on linux to linux-gnu, so seven fail on the Android CI and two natively. They are now explicitly excluded. - swiftlang#39605 added several C++ Interop tests, 11 of which fail on the Android CI, so disable them for now. - swiftlang#42478 removed the @NoEscape attribute for the non-Android SIL/clang-function-types tests, so I remove it for Android too. - My pull swiftlang#40779 moved the Swift pointer tags to the second byte, so SILOptimizer/concat_string_literals.64 will need to be updated for that, disabled it for now. - Compiler-rt moved the directory in which it places those libraries for Android, llvm/llvm-project@a68ccba, so lit.cfg is updated for that.

zoecarver added the c++ interop Feature: Interoperability with C++ label Oct 6, 2021

zoecarver commented Oct 6, 2021

View reviewed changes

test/Interop/Cxx/foreign-reference/Inputs/pod.h Show resolved Hide resolved

rjmccall requested changes Oct 6, 2021

View reviewed changes

zoecarver force-pushed the cxx-interop-import-as-class branch 2 times, most recently from 6cf5f8e to dd2f450 Compare October 13, 2021 17:18

zoecarver marked this pull request as ready for review October 13, 2021 17:19

zoecarver force-pushed the cxx-interop-import-as-class branch from dd2f450 to b693fc7 Compare October 13, 2021 17:40

zoecarver requested a review from rjmccall October 13, 2021 17:43

guitard0g reviewed Oct 15, 2021

View reviewed changes

lib/SIL/Verifier/SILVerifier.cpp Outdated Show resolved Hide resolved

zoecarver force-pushed the cxx-interop-import-as-class branch 2 times, most recently from 5a89572 to 65b4f3c Compare October 22, 2021 00:13

zoecarver mentioned this pull request Oct 28, 2021

[libswift] Refactor libSwift to use SIL types directly and remove bridged versions. #39958

Open

zoecarver force-pushed the cxx-interop-import-as-class branch from 65b4f3c to 89fc96a Compare November 19, 2021 22:46

zoecarver commented Nov 20, 2021

View reviewed changes

zoecarver force-pushed the cxx-interop-import-as-class branch from 89fc96a to 191800f Compare November 20, 2021 00:48

zoecarver requested review from beccadax, egorzhdan and hyp November 20, 2021 00:49

zoecarver force-pushed the cxx-interop-import-as-class branch 2 times, most recently from 6e17e51 to ed9dad3 Compare November 29, 2021 20:58

zoecarver force-pushed the cxx-interop-import-as-class branch 2 times, most recently from 6feca79 to 298cb75 Compare December 4, 2021 01:23

zoecarver force-pushed the cxx-interop-import-as-class branch 2 times, most recently from c6f92e4 to 815b95a Compare December 5, 2021 19:02

zoecarver force-pushed the cxx-interop-import-as-class branch from 815b95a to 649b072 Compare December 5, 2021 19:18

zoecarver force-pushed the cxx-interop-import-as-class branch from 649b072 to f01913e Compare December 6, 2021 08:21

zoecarver force-pushed the cxx-interop-import-as-class branch from f01913e to 58d5f52 Compare December 6, 2021 16:32

zoecarver force-pushed the cxx-interop-import-as-class branch from 58d5f52 to 817a695 Compare December 6, 2021 22:25

zoecarver force-pushed the cxx-interop-import-as-class branch from 817a695 to 9f0119b Compare December 6, 2021 22:28

zoecarver force-pushed the cxx-interop-import-as-class branch from 9f0119b to c38d1ed Compare December 8, 2021 10:17

zoecarver force-pushed the cxx-interop-import-as-class branch from c38d1ed to ae9b541 Compare December 8, 2021 11:59

[cxx-interop] Implement foreign reference types.

fc3b3a1

This is an expiremental feature to allow an attribute, `import_as_ref`, to import a C++ record as a non-reference-counted reference type in Swift.

zoecarver force-pushed the cxx-interop-import-as-class branch from ae9b541 to fc3b3a1 Compare December 8, 2021 15:35

hyp reviewed Dec 8, 2021

View reviewed changes

zoecarver commented Dec 8, 2021

View reviewed changes

hyp approved these changes Dec 8, 2021

View reviewed changes

zoecarver merged commit a46a3c5 into swiftlang:main Dec 8, 2021

bro4all self-requested a review December 9, 2021 06:06

finagolfin mentioned this pull request Dec 19, 2021

[android] Remove ICU build flags since that requirement was dropped in #40340 #40625

Merged

finagolfin mentioned this pull request Jun 9, 2022

[android][test] Fix several tests on the Android CI #59279

Merged

Conversation

zoecarver commented Oct 6, 2021

Uh oh!

Uh oh!

rjmccall Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

zoecarver Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zoecarver commented Oct 13, 2021

Uh oh!

Uh oh!

zoecarver commented Oct 22, 2021

Uh oh!

zoecarver commented Oct 22, 2021

Uh oh!

zoecarver commented Oct 25, 2021

Uh oh!

zoecarver commented Oct 28, 2021

Uh oh!

zoecarver Nov 20, 2021

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Nov 20, 2021

Uh oh!

zoecarver commented Nov 29, 2021

Uh oh!

zoecarver commented Nov 29, 2021

Uh oh!

zoecarver commented Dec 5, 2021

Uh oh!

zoecarver commented Dec 5, 2021

Uh oh!

zoecarver commented Dec 5, 2021

Uh oh!

zoecarver commented Dec 6, 2021

Uh oh!

zoecarver commented Dec 6, 2021

Uh oh!

zoecarver commented Dec 6, 2021

Uh oh!

zoecarver commented Dec 6, 2021

Uh oh!

zoecarver commented Dec 8, 2021

Uh oh!

zoecarver commented Dec 8, 2021

Uh oh!

zoecarver commented Dec 8, 2021

Uh oh!

hyp Dec 8, 2021

Choose a reason for hiding this comment

Uh oh!

zoecarver Dec 8, 2021

Choose a reason for hiding this comment

Uh oh!

zoecarver Dec 8, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants