-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[cxx-interop] Allow importing templated functions when template args do not appear in function signature #39535
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
Conversation
|
WIP because there seems to be some wonkiness with how this interacts with default template arguments, still investigating a fix there. |
zoecarver
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a great patch! Thank you, Josh.
I really like how you commented all the code you added so it's super easy to understand and follow. Also, thanks for factoring out that lambda into a general helper function.
I have one or two small nits, but overall, this patch seems good to me :)
lib/ClangImporter/ImportType.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's link this to the SR: XXXX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, meant to actually give you the SR. Just update the TODO to read TODO(SR-13809):. This way we can grep for the SR and find all the places we need to update whenever we add support for dependent types.
lib/Sema/CSApply.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I named this function, but it's kind of ugly, so while you're updating the signature maybe change it to generateSpecializedCXXFunctionTemplate? WDYT?
lib/Sema/CSApply.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in this patch, but at some point we should move all synthesizers to their own file. They just take up space right now and clutter the files they're scattered across.
test/Interop/Cxx/templates/Inputs/template-type-parameter-not-in-signature.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an expectEquals somewhere in both of these tests. Otherwise (very theoretically), they might just get optimized away. Also it's not a bad idea to just test that the correct thing is returned and there wasn't an argument mismatch or something.
|
@guitard0g let me know if you need help running the bots. |
egorzhdan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach looks great to me! I only noticed one small issue that prevents the C++ stdlib from being imported.
2fa8285 to
d81a5c3
Compare
|
@swift-ci please test |
d81a5c3 to
d2c68cc
Compare
|
@swift-ci please test |
|
@egorzhdan @zoecarver Thanks for the review! I believe I've addressed all the comments in the most recent revision. It changed a decent bit because I realized there were some quirks with making sure we manually invoke the generic substitutions on the signature of the synthesized thunk. Should be ready for another round of reviews. |
zoecarver
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢 it
(I left three very small nits. All are optional.)
lib/Sema/CSApply.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we could use auto [a, b] = 😢
lib/Sema/CSApply.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think various methods/static methods might take a meta type. We need to verify if this will break that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are passing, so I guess this isn't an issue.
Now you see why I'm so excited about #36082 :P |
egorzhdan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
in the function signature by adding explicit metatype parameters to the function signature.
d2c68cc to
f433ac2
Compare
|
@swift-ci please smoke test and merge |
Currently if we try to import a templated function when the template parameters do not appear in the function signature, the corresponding Swift function will have extra unused generic parameters, which means we can't call those functions. This patch fixes the problem by making those template params into explicit metatype function parameters to the corresponding Swift function. E.g.
becomes:
instead of:
This extra argument is only used to deduce the generic type needed to specialize the C++ function. The corresponding function call made under the hood will cut out the extra argument and forward along the proper parameters to the C++ function.