Skip to content

[cxx-interop] Add SIL function representation cxx_method; Support extending C++ types. #34993

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
Jan 7, 2022
Merged

[cxx-interop] Add SIL function representation cxx_method; Support extending C++ types. #34993

merged 1 commit into from
Jan 7, 2022

Conversation

zoecarver
Copy link
Contributor

@zoecarver zoecarver commented Dec 8, 2020 •
edited
Loading

There are three major changes here:

  1. The addition of SILFunctionTypeRepresentation::CXXMethod.
  2. C++ methods are imported with their members last. Then the arguments are switched when emitting the IR for an application of the function.
  3. Clang decls are now marked as foreign witnesses.

These are all steps towards being able to have C++ protocol conformance.

A few other notes:

  • There is a surprising lack of C++ method tests. As a separate PR, I'll add some shortly.
  • This will also let me write some tests for the witness layout optimizations (as requested here).
  • Protocol conformance is also needed for some future generics work.
  • I'm hoping to submit future patches to add support for static C++ methods and operators.

Resolves SR-12750.

@zoecarver zoecarver added the c++ interop Feature: Interoperability with C++ label Dec 8, 2020
varungandhi-apple
varungandhi-apple reviewed Dec 8, 2020
View reviewed changes
varungandhi-apple
varungandhi-apple reviewed Dec 8, 2020
View reviewed changes
varungandhi-apple
varungandhi-apple reviewed Dec 8, 2020
View reviewed changes
varungandhi-apple
varungandhi-apple reviewed Dec 8, 2020
View reviewed changes
varungandhi-apple
varungandhi-apple reviewed Dec 8, 2020
View reviewed changes
@zoecarver zoecarver force-pushed the cxx/cxxmethod-representation branch from f20dd85 to e21aa57 Compare December 8, 2020 18:27
@zoecarver
Copy link
Contributor Author

@varungandhi-apple all resolved. Thanks for the review!

varungandhi-apple
varungandhi-apple reviewed Dec 8, 2020
View reviewed changes
@zoecarver zoecarver force-pushed the cxx/cxxmethod-representation branch from e21aa57 to cd9e0bc Compare December 8, 2020 18:32
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

1 similar comment
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor Author

@swift-ci please test Windows.

@varungandhi-apple
Copy link
Contributor

Mostly LGTM, deferring to other folks for specifics.

@zoecarver zoecarver force-pushed the cxx/cxxmethod-representation branch from cd9e0bc to 686dcb2 Compare December 9, 2020 19:31
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

1 similar comment
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver zoecarver force-pushed the cxx/cxxmethod-representation branch from 686dcb2 to 7b839bc Compare December 10, 2020 18:10
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor Author

@rjmccall and @hlopko friendly ping.

@zoecarver zoecarver mentioned this pull request Jan 22, 2021
Merged
@zoecarver zoecarver requested a review from slavapestov February 1, 2021 00:06
@zoecarver zoecarver mentioned this pull request Feb 24, 2021
Merged
hyp
hyp reviewed Jan 4, 2022
View reviewed changes
lib/IRGen/GenCall.cpp Outdated
@@ -2187,7 +2191,12 @@ class SyncCallEmission final : public CallEmission {
break;

case SILFunctionTypeRepresentation::Block:
adjusted.add(getCallee().getBlockObject());
case SILFunctionTypeRepresentation::CXXMethod:
if (getCallee().getRepresentation() ==
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you handling the block and cxxmethod in same case if you need to do a different call for block vs cxx method? I think it would be better to have separate cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha. Turns out there was actually a reason for this: we want the fall-through here for both cases. Without this weird if-inside-the-case pattern we don't externalizeArguments for ::Block.

hyp
hyp reviewed Jan 4, 2022
View reviewed changes
hyp
hyp reviewed Jan 4, 2022
View reviewed changes
hyp
hyp reviewed Jan 4, 2022
View reviewed changes
hyp
hyp reviewed Jan 4, 2022
View reviewed changes
@zoecarver zoecarver force-pushed the cxx/cxxmethod-representation branch 2 times, most recently from 616fb73 to 092375e Compare January 5, 2022 00:18
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver zoecarver force-pushed the cxx/cxxmethod-representation branch from 092375e to 12c337d Compare January 5, 2022 16:59
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver zoecarver requested a review from hyp January 5, 2022 17:00
@zoecarver zoecarver force-pushed the cxx/cxxmethod-representation branch from 12c337d to a6de933 Compare January 5, 2022 18:32
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver zoecarver requested a review from egorzhdan January 5, 2022 19:35
@zoecarver zoecarver force-pushed the cxx/cxxmethod-representation branch from a6de933 to fcb1f7e Compare January 6, 2022 22:26
@zoecarver
[cxx-interop] Add SIL function representation cxx_method; Support ext…
036361d 
…ending C++ types.

There are three major changes here:
    1. The addition of "SILFunctionTypeRepresentation::CXXMethod".
    2. C++ methods are imported with their members *last*. Then the arguments are switched when emitting the IR for an application of the function.
    3. Clang decls are now marked as foreign witnesses.

These are all steps towards being able to have C++ protocol conformance.
@zoecarver zoecarver force-pushed the cxx/cxxmethod-representation branch from fcb1f7e to 036361d Compare January 6, 2022 22:26
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

slavapestov
slavapestov reviewed Jan 6, 2022
View reviewed changes
lib/SILGen/SILGenType.cpp
// as foreign.
if (dyn_cast_or_null<clang::CXXMethodDecl>(
witness.getDecl()->getClangDecl()))
newDecl = newDecl.asForeign();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are C++ methods different than Objective-C methods in this regard?

Copy link
Contributor Author

@zoecarver zoecarver Jan 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Basically, if an Objective-C method is marked as foreign it will be imported using the Objective-C type and not the Swift type. For example, it might be imported as () -> NSString instead of what we want which is: () -> String.

hyp
hyp approved these changes Jan 7, 2022
View reviewed changes
Copy link
Contributor

@hyp hyp left a comment

Choose a reason for hiding this comment

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

LGTM

@zoecarver zoecarver merged commit 53ab3ef into swiftlang:main Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slavapestov slavapestov slavapestov left review comments

@varungandhi-apple varungandhi-apple varungandhi-apple left review comments

@hyp hyp hyp approved these changes

@hlopko hlopko Awaiting requested review from hlopko

@gribozavr gribozavr Awaiting requested review from gribozavr

@martinboehme martinboehme Awaiting requested review from martinboehme

@nate-chandler nate-chandler Awaiting requested review from nate-chandler

@rjmccall rjmccall Awaiting requested review from rjmccall rjmccall is a code owner

@egorzhdan egorzhdan Awaiting requested review from egorzhdan egorzhdan is a code owner

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.
4 participants
@zoecarver @varungandhi-apple @slavapestov @hyp