Skip to content

C++ Interop: improve skipping already imported struct members #38439

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

Merged
merged 1 commit into from
Jul 19, 2021
Merged

C++ Interop: improve skipping already imported struct members #38439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions lib/ClangImporter/ImportDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3501,6 +3501,10 @@ namespace {
SmallVector<ConstructorDecl *, 4> ctors;
SmallVector<TypeDecl *, 4> nestedTypes;

// Store the already imported members in a set to avoid importing the same
// decls multiple times.
SmallPtrSet<clang::Decl *, 16> importedMembers;

// FIXME: Import anonymous union fields and support field access when
// it is nested in a struct.

Expand All @@ -3519,6 +3523,12 @@ namespace {
}
}

// If we've already imported this decl & added it to the resulting
// struct, skip it so we don't add the same member twice.
if (!importedMembers.insert(m->getCanonicalDecl()).second) {
continue;
}

auto nd = dyn_cast<clang::NamedDecl>(m);
if (!nd) {
// We couldn't import the member, so we can't reference it in Swift.
Expand Down Expand Up @@ -3547,12 +3557,6 @@ namespace {
}
}

// If we've already imported this decl, skip it so we don't add the same
// member twice.
if (Impl.ImportedDecls.find({nd->getCanonicalDecl(), getVersion()}) !=
Impl.ImportedDecls.end())
continue;

// If we encounter an IndirectFieldDecl, ensure that its parent is
// importable before attempting to import it because they are dependent
// when it comes to getter/setter generation.
Expand Down
5 changes: 5 additions & 0 deletions test/Interop/Cxx/namespace/Inputs/module.modulemap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,8 @@ module TemplatesSecondHeader {
header "templates-second-header.h"
requires cplusplus
}

module TemplatesWithForwardDecl {
header "templates-with-forward-decl.h"
requires cplusplus
}
37 changes: 37 additions & 0 deletions test/Interop/Cxx/namespace/Inputs/templates-with-forward-decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#ifndef TEST_INTEROP_CXX_NAMESPACE_INPUTS_TEMPLATES_WITH_FORWARD_DECL_H
#define TEST_INTEROP_CXX_NAMESPACE_INPUTS_TEMPLATES_WITH_FORWARD_DECL_H

namespace NS1 {

template <typename T>
Copy link
Contributor

Choose a reason for hiding this comment

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

Come to think of it, I bet you could get this behavior without using a template. Can you please try that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm that's weird, but it doesn't seem reproducible without a template. I'll need a bit more time to figure out why.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is interesting. It would be good to understand why we need this to be a template. But in the mean time, I don't think this patch should be blocked on that.

struct Decl;

typedef Decl<int> di;

} // namespace NS1

namespace NS1 {

template <typename T>
struct ForwardDeclared;

template <typename T>
struct Decl {
typedef T MyInt;
ForwardDeclared<T> fwd;
const static MyInt intValue = -1;
};

template <typename T>
const typename Decl<T>::MyInt Decl<T>::intValue;

} // namespace NS1

namespace NS1 {

template <typename T>
struct ForwardDeclared {};

} // namespace NS1

#endif // TEST_INTEROP_CXX_NAMESPACE_INPUTS_TEMPLATES_WITH_FORWARD_DECL_H
9 changes: 9 additions & 0 deletions test/Interop/Cxx/namespace/templates-with-forward-decl-module-interface.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// RUN: %target-swift-ide-test -print-module -module-to-print=TemplatesWithForwardDecl -I %S/Inputs -source-filename=x -enable-cxx-interop | %FileCheck %s

// CHECK: extension NS1 {
// CHECK: struct __CxxTemplateInstN3NS14DeclIiEE {
// CHECK: typealias MyInt = Int32
// CHECK: var fwd: NS1.__CxxTemplateInstN3NS115ForwardDeclaredIiEE
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 check these class template specializations too? Might as well, these module interface tests are pretty easy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amusingly, we would need #37695 for that :) Currently the lookup logic gets confused & only the contents of the first NS1 redecl end up in the module interface. Applying #37695 locally solves this (all the contents get included into the module interface), but I guess it would be better to get this PR merged first, so it's too early to add a // CHECK for those decls now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Maybe if you think of it, you could fix this as a follow up. But it's not super important. The test is good as is.

// CHECK: static let intValue: NS1.__CxxTemplateInstN3NS14DeclIiEE.MyInt
// CHECK: }
// CHECK: }