[cxx-interop] Avoid emitting methods that cause name clash #74250
Conversation
In Swift, we can have enum elements and methods with the same name, they are overloaded. Unfortunately, this feature is not supported by C++ interop at the moment. This patch avoids emitting the methods with name collisions to make sure the resulting header can be compiler. A proper fix should follow in a later PR. rdar://128162252
| for (const auto *enumElement : enumDecl->getAllElements()) { | ||
| auto elementName = enumElement->getName(); | ||
| if (!elementName.isSpecial() && elementName.getBaseIdentifier().str() == name) | ||
| return ClangRepresentation::unsupported; |
There was a problem hiding this comment.
Unfortunately, with the current approach, we sometimes already write some strings to the ostream (that writes to the header) by the time we realise that a construct is unsupported. In this particular case we only added two spaces, so it should not cause any trouble (other than formatting).
There are other code paths below returning unsupported after the declaration is already partially written to the stream which is bad. I think this should be fixed independently of this PR, probably by writing the declaration to a temporary buffer, and commit/write that buffer to the output stream at the very end.
| // does not compiler. | ||
| // FIXME: either do not emit cases as inline members, or rename the cases or the | ||
| // colliding functions. | ||
| for (const auto *enumElement : enumDecl->getAllElements()) { |
There was a problem hiding this comment.
I am not sure how performance sensitive is this code path. Note that, this could potentially be "quadratic": num(elements)*num(methods). An asymptotically better behaving solution might do some kinds of lookup by name.
| auto result = printFunctionSignature( | ||
| FD, signature, cxx_translation::getNameForCxx(FD), resultTy, | ||
| FunctionSignatureKind::CxxInlineThunk, modifiers); | ||
| assert(!result.isUnsupported() && "C signature should be unsupported too"); |
There was a problem hiding this comment.
There are multiple code paths in printFunctionSignature that already return unsupported, even before this PR. I think we should handle that scenario gracefully here.
|
@swift-ci please smoke test |
egorzhdan
left a comment
egorzhdan
left a comment
There was a problem hiding this comment.
Thanks! Just a couple of small comments.
|
@swift-ci please smoke test |
2 participants
In Swift, we can have enum elements and methods with the same name, they are overloaded. Unfortunately, this feature is not supported by C++ interop at the moment. This patch avoids emitting the methods with name collisions to make sure the resulting header can be compiler. A proper fix should follow in a later PR.
rdar://128162252