Skip to content

[cxx-interop] Skip type metadata for C++ types that are only used in private C++ fields #78467

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.

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
Jan 13, 2025

Conversation

egorzhdan
Copy link
Contributor

This fixes compiler errors for C++ types that use pimpl idiom:

invalid application of 'sizeof' to an incomplete type

rdar://141960396

@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label Jan 7, 2025
@egorzhdan egorzhdan requested a review from Xazax-hun January 7, 2025 17:32
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan egorzhdan force-pushed the egorzhdan/sizeof-incomplete branch from 6542187 to a0d24fb Compare January 7, 2025 19:50
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test Linux

2 similar comments
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test Linux

@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test Linux

@egorzhdan egorzhdan marked this pull request as ready for review January 9, 2025 12:52
@egorzhdan egorzhdan requested a review from rjmccall as a code owner January 9, 2025 12:52
@rjmccall
Copy link
Contributor

rjmccall commented Jan 10, 2025

LGTM, but please add a test case; it should be straightforward to test that we don't emit field metadata for these fields.

Also, is the right condition that it's private or that it isn't protected?

@egorzhdan
Copy link
Contributor Author

Thanks @rjmccall, I'll add a test before merging this.

Also, is the right condition that it's private or that it isn't protected?

We're checking the access level of the imported Swift decl, and C++ protected fields get imported as private in Swift, so the condition should be correct.

@egorzhdan egorzhdan force-pushed the egorzhdan/sizeof-incomplete branch from a0d24fb to b31a282 Compare January 10, 2025 18:34
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

…private C++ fields

This fixes compiler errors for C++ types that use pimpl idiom:
```
invalid application of 'sizeof' to an incomplete type
```

rdar://141960396
@egorzhdan egorzhdan force-pushed the egorzhdan/sizeof-incomplete branch from b31a282 to 738c8fb Compare January 13, 2025 12:04
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan egorzhdan merged commit 18be5e8 into main Jan 13, 2025
3 checks passed
@egorzhdan egorzhdan deleted the egorzhdan/sizeof-incomplete branch January 13, 2025 16:58
tbkka added a commit to tbkka/swift that referenced this pull request Jan 27, 2025
PR swiftlang#78467 omitted certain fields from the FieldDescriptor list,
but did not update the count of fields that's emitted
just before that list.

Update the count so it matches the number of field descriptors
we actually emit.

Resolves rdar://143402921
tbkka added a commit to tbkka/swift that referenced this pull request Jan 28, 2025
PR swiftlang#78467 omitted certain fields from the FieldDescriptor list,
but did not update the count of fields that's emitted
just before that list.

Update the count so it matches the number of field descriptors
we actually emit.

Resolves rdar://143402921
susmonteiro added a commit that referenced this pull request May 23, 2025
In #78467 and #78961, we stopped emitting metadata for private C++ fields. However, this created a mismatch between the fields emitted and the number of fields + their offsets in the StructDescriptor.

rdar://147263490
(cherry picked from commit 72b13b3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants