IRGen: Only skip round-trip demangler check for types that involve C++ types #85288
Conversation
slavapestov
force-pushed
the
narrow-cxx-roundtrip-carveout
branch
from
629a466 to
0be5386
Compare
|
@swift-ci Please smoke test |
egorzhdan
left a comment
egorzhdan
left a comment
There was a problem hiding this comment.
This check doesn't seem diligent enough as is. In general, detecting if something is a C++-only type is a difficult problem that we try to sidestep in other parts of the compiler. I think we might be able to sidestep it here as well – I left an inline comment.
The goal is not to precisely detect C++-only types, but just those types for which ASTDemangler support is not implemented. Simply disabling the round-trip check for all imported types is not acceptable, since those have to work too. Of course, we could also fix the ASTDemangler. |
slavapestov
force-pushed
the
narrow-cxx-roundtrip-carveout
branch
from
0be5386 to
fa38b60
Compare
slavapestov
force-pushed
the
narrow-cxx-roundtrip-carveout
branch
from
fa38b60 to
a9a478c
Compare
…+ types Previously this was disabled entirely if C++ interop was enabled, which is too broad.
slavapestov
force-pushed
the
narrow-cxx-roundtrip-carveout
branch
from
a9a478c to
d486919
Compare
|
@swift-ci Please smoke test |
| // a private discriminator, in which case this entry point will be called | ||
| // with the right parameters so that this isn't needed. | ||
| return (VD->getFormalAccess() <= AccessLevel::FilePrivate && | ||
| !VD->hasClangNode()); |
There was a problem hiding this comment.
I'm concerned that this won't work for inherited members like methods and fields.
We import those as a synthesized Swift shim that calls a synthesized C++ forwarding function that accesses the underlying inherited member (doing so isolates Swift from some of the nuances of C++ inheritance, whose logic we'd like to not duplicate from Clang).
However, the synthesized Swift shim is not associated with any Clang node, and will thus be missed by the check here. And I don't think we have any mechanism for saying "this node was imported from Clang but I don't have a Clang node pointer to show for it."
I'm wondering if the right thing to do here is to just introduce a private discriminator for anything imported from C++? And what would be the requirements for such a discriminator? All non-public decls in C++ are members of record types, i.e., not top-level, so I wonder if it suffices to just have Clang module units to just all share __ObjC or something like that as their private discriminator.
There was a problem hiding this comment.
Since the round-trip check is only enabled in asserts builds by default, can we leave that for a follow-up PR? I agree that fixing the private discriminator will allow this part of the hack to be removed.
There was a problem hiding this comment.
Yeah, that sounds good to me!
3 participants
Previously this was disabled entirely if C++ interop was enabled, which is too broad.