Skip to content

[Cxx-Interop] Enable Record Friend functions #59801

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 8 commits into from
Jul 6, 2022
Merged

[Cxx-Interop] Enable Record Friend functions #59801

merged 8 commits into from
Jul 6, 2022

Conversation

Huddie
Copy link
Contributor

@Huddie Huddie commented Jun 30, 2022 •
edited
Loading

Records may have friend functions (like operators) which do not get imported at all at the moment.

This is because friends are nested inside of a clang::FriendDecl rather than being a normal clang::Function for example.
Although the clang::FriendDecl may live inside a CXXRecord context, the "friend" (nested decl -- such as clang::FunctionDecl) live inside of a TranslationUnit context.

This PR adds the ability to import Friend Functions that live inside of Records. Can be expanded later to include other decl types and those that live outside of records as well.

Looking to get some feedback on better ways of informing the compiler later on that this record (clang::FunctionDecl) came from a clang::FriendDecl (since its been "unwrapped" already the compiler no longer knows that it came from a clang::FriendDecl, all it knows is that its clang::FunctionDecl) This is made difficult by the fact that getDeclContext() returns TranslationUnit on the clang::FunctionDecl, not clang::FriendDecl

Still needs:

  • tests

Issue identified: #59492 (comment)

@Huddie Huddie requested a review from zoecarver June 30, 2022 00:26
@Huddie
Copy link
Contributor Author

Huddie commented Jun 30, 2022

@swift-ci please smoke test

Huddie
Huddie commented Jun 30, 2022
View reviewed changes
lib/ClangImporter/ImportDecl.cpp Outdated
if (auto friendDecl = dyn_cast<clang::FriendDecl>(m)) {
auto d = VisitFriendDecl(friendDecl);

// Check if the friend decl was converted into a FuncDecl
Copy link
Contributor Author

@Huddie Huddie Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should probably remove this comment and add doc-string to VisitFriendDecl saying that this function returns the Swift imported version of the "friend" (nested) decl found inside this clang::FriendDecl

egorzhdan
egorzhdan reviewed Jun 30, 2022
View reviewed changes
Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

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

This is great!

  1. Please run clang-format on all of the changed code fragments (cmd+opt+L in your setup).
  2. Please remove the empty-line changes.
  3. Can you please add a few tests to check that e.g. friend bool operator== gets imported? You could add a couple of friend operator declarations to member-inline.h and a couple of extra checks in member-inline-module-interface.swift & member-inline.swift.

zoecarver
zoecarver reviewed Jun 30, 2022
View reviewed changes
lib/ClangImporter/ImportDecl.cpp Outdated
@@ -2116,6 +2116,16 @@ namespace {
continue;
}

if (auto friendDecl = dyn_cast<clang::FriendDecl>(m)) {
if (auto d = VisitFriendDecl(friendDecl)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be done automatically on line 2147:

Decl *member = Impl.importDecl(nd, getActiveSwiftVersion());

Is there some reason that doesn't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be! Testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FriendDecl is not a NamedDecl so it does not get to that line.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about my above suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should work, would have to add to result->addMember later on. But offline sounds like maybe we should make this global

zoecarver
zoecarver reviewed Jun 30, 2022
View reviewed changes
lib/ClangImporter/ImportDecl.cpp Outdated
@@ -3044,6 +3054,16 @@ namespace {
result->setIsObjC(false);
result->setIsDynamic(false);

bool isFromFriend = Impl.cxxFriends.contains(decl);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put this in the importFunctionDecl method? Can we just look at the decl context instead of Impl.cxxFriends.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cant look at declContext because the passed in decl is clang::FunctionDecl and its context is not clang::FriendDecl but rather TranslationUnit.

Copy link
Contributor

Choose a reason for hiding this comment

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

The aptly named clang::FunctionDecl:: isThisDeclarationInstantiatedFromAFriendDefinition :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whhhahaaaattttt 🤯

egorzhdan
egorzhdan reviewed Jun 30, 2022
View reviewed changes
zoecarver
zoecarver reviewed Jul 1, 2022
View reviewed changes
lib/ClangImporter/ImportDecl.cpp
@@ -2116,6 +2116,16 @@ namespace {
continue;
}

if (auto friendDecl = dyn_cast<clang::FriendDecl>(m)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (auto friendDecl = dyn_cast<clang::FriendDecl>(m)) {
if (auto friendDecl = dyn_cast<clang::FriendDecl>(m)) {
m = friendDecl->getFriend();

@Huddie
Copy link
Contributor Author

Huddie commented Jul 3, 2022

@swift-ci please smoke test

zoecarver
zoecarver approved these changes Jul 4, 2022
View reviewed changes
Copy link
Contributor

@zoecarver zoecarver left a comment

Choose a reason for hiding this comment

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

Looks great. Much simpler. We might have to add another special case to the module printer to get the IDE tests working (that can be a follow up PR).

@Huddie
Copy link
Contributor Author

Huddie commented Jul 4, 2022

@swift-ci please smoke test

@Huddie
Copy link
Contributor Author

Huddie commented Jul 5, 2022

@swift-ci please smoke test

@Huddie
Copy link
Contributor Author

Huddie commented Jul 5, 2022

@swift-ci please smoke test linux

@Huddie
Copy link
Contributor Author

Huddie commented Jul 6, 2022

@swift-ci please smoke test

egorzhdan
egorzhdan approved these changes Jul 6, 2022
View reviewed changes
Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@Huddie Huddie merged commit c6e89b9 into swiftlang:main Jul 6, 2022
@Huddie Huddie added the c++ interop Feature: Interoperability with C++ label Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egorzhdan egorzhdan egorzhdan approved these changes

@zoecarver zoecarver zoecarver approved these changes

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.
3 participants
@Huddie @egorzhdan @zoecarver