[cxx-interop] Support forward declared records inside other records. #34864
Conversation
zoecarver
force-pushed
the
cxx/fix/friends
branch
from
5556f86 to
e359928
Compare
|
@swift-ci please smoke test |
lib/ClangImporter/ImportDecl.cpp
Outdated
| if (auto record = dyn_cast<clang::RecordDecl>(nd)) { | ||
| // If this record only being declared inside of "decl" then simply | ||
| // ignore it because it's a forward declaration. | ||
| if (record->getDefinition() != record) |
There was a problem hiding this comment.
Could you add a test for forward declaration without definition?
Are you only interested in skipping friend declarations? If so we could make the check more specific.
Will your change do the right thing (which I guess is importing the definition) for this snippet?
struct Foo {
struct Bar;
};
struct Foo::Bar {
int a;
};There was a problem hiding this comment.
I'll keep the current test and add this one. I think it's beneficial to have both because the friend declaration isn't scoped.
There was a problem hiding this comment.
I tried adding that test but it, unfortunately, requires #34869. Once that lands, I'll update this patch with your suggested test.
|
After rebasing this is now a test-only change. |
|
This is now crashing in a different place. #35085 should fix the new crash, so I'm just going to wait for that to land. |
zoecarver
force-pushed
the
cxx/fix/friends
branch
from
e359928 to
21b1338
Compare
|
|
||
| struct ForwardDeclaresFriend { | ||
| friend struct ForwardDeclaredFriend; | ||
| friend void takesFriend(struct ForwardDeclaredFriend f); |
There was a problem hiding this comment.
| friend void takesFriend(struct ForwardDeclaredFriend f); | |
| friend void takesFriend(ForwardDeclaredFriend f); |
Not needed in C++.
There was a problem hiding this comment.
I know. This was part of the reproducer, IIRC it didn't crash without it. I can't verify this now that it no longer crashes, but I think we should keep it.
…rds.
The pattern:
struct X { friend struct Y; }; struct Y {};
is fairly common. But before this commit we would crash while attempting
to add "Y" as a child of "X". This commit simply checks if the child
record is a declaration or definition. If the former, it bails (and the
"child" record will be imported where it's defined).
zoecarver
force-pushed
the
cxx/fix/friends
branch
from
21b1338 to
f6a31e0
Compare
|
@swift-ci please smoke test. |
|
@swift-ci please smoke test macOS. |
|
@swift-ci please test Windows. |
|
Another Java error. @swift-ci please test Windows. |
|
The windows test failed because of an unrelated error. I'm going to go ahead and land this because it's just a NFC and it passed on the windows CI (even though another, unrelated test failed). |
The pattern:
is fairly common. But before this commit, we would crash while attempting to add "Y" as a child of "X". This commit simply checks if the child record is a declaration or definition. If the former, it bails (and the "child" record will be imported where it's defined).