C++ Interop: improve skipping already imported struct members #38439
Conversation
|
@swift-ci please smoke test |
There was a problem hiding this comment.
This LGTM. I think you have the infra setup locally to import the standard library. I assume you've already done this, but can you please make sure we are still able to import the standard library without crashing after this patch is applied? (We should really have this be part of the CI/test suite now...)
| // CHECK: extension NS1 { | ||
| // CHECK: struct __CxxTemplateInstN3NS14DeclIiEE { | ||
| // CHECK: typealias MyInt = Int32 | ||
| // CHECK: var fwd: NS1.__CxxTemplateInstN3NS115ForwardDeclaredIiEE |
There was a problem hiding this comment.
Can we check these class template specializations too? Might as well, these module interface tests are pretty easy.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
lib/ClangImporter/ImportDecl.cpp
Outdated
| @@ -3578,6 +3573,12 @@ namespace { | |||
| continue; | |||
| } | |||
|
|
|||
| // If we've already added this decl to the resulting struct, skip it so | |||
| // we don't add the same member twice. | |||
| if (!allMembers.insert(member).second) { | |||
There was a problem hiding this comment.
We don't want to do this before L3570/3565 when we try to import the decl?
There was a problem hiding this comment.
Hmm yes, that makes sense. Fixed.
|
Just so we're on the same page here, the problem is:
|
|
|
||
| namespace NS1 { | ||
|
|
||
| template <typename T> |
There was a problem hiding this comment.
Come to think of it, I bet you could get this behavior without using a template. Can you please try that :)
There was a problem hiding this comment.
Hmm that's weird, but it doesn't seem reproducible without a template. I'll need a bit more time to figure out why.
There was a problem hiding this comment.
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.
This fixes the issue that prevents `std::string::size_type` from being imported into Swift: `size_type` is imported before its parent struct, and we're skipping it when importing the struct afterwards.
This is caused by an out-of-line decl for `std::string::npos`:
```cpp
template<class _CharT, class _Traits, class _Allocator>
_LIBCPP_FUNC_VIS
const typename basic_string<_CharT, _Traits, _Allocator>::size_type
basic_string<_CharT, _Traits, _Allocator>::npos;
```
When importing `npos`, we first import `size_type`, which triggers the issue.
egorzhdan
force-pushed
the
cxx-string-npos
branch
from
30a4f7d to
fc2dc6a
Compare
Yeap, I've tried it and it works for me, definitely doesn't crash.
Absolutely, I'm waiting for #37806 to land, and after that I'll submit a patch with a test for stdlib import.
Not quite (my bad, I should've described this in more detail)
|
|
@swift-ci please smoke test |
This fixes the issue that prevents
std::string::size_typefrom being imported into Swift:size_typeis imported before its parent struct, and we're skipping it when importing the struct afterwards.This is caused by an out-of-line decl for
std::string::npos:When importing
npos, we first importsize_type, which triggers the issue.