-
Notifications
You must be signed in to change notification settings - Fork 10.5k
C++ Interop: import subscript operators #36365
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
@swift-ci please smoke test |
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.
First, this looks really great! Thank you for all your work on importing operators.
I've left some comments here but I'm not done reviewing this. I'd also like to find someone to review the subscript expression synthesis logic, I don't know much about that code. Maybe @slavapestov can help.
lib/ClangImporter/ImportDecl.cpp
Outdated
@@ -3639,6 +3639,29 @@ namespace { | |||
return nullptr; | |||
} | |||
} | |||
|
|||
for (const auto method : methods) { |
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'm not sure this is the correct place for this logic. Why not do this when importing the method itself?
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.
If we were able to combine this with the logic for importing methods, then we could also get rid of cxxSubscripts
, I think.
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.
Basically having a single operator[]
method isn't always enough to generate a subscript. I don't think we can easily get rid of cxxSubscripts
, here's why:
Imagine we're importing a const operator[]
with a parameter type that wasn't used in any of the already imported operator[]
methods. This method always corresponds to a subscript getter, and a subscript with this getter is always valid, so we could just add a getter-only subscript and move on, and later if we import a non-const operator[]
with the same parameter type, we'll add a setter to the existing subscript.
But if we get a non-const operator[]
with an unseen before parameter type, there's no obvious way to proceed. We can't add a subscript right away, because setter-only subscripts aren't valid in Swift. We could create a new subscript with both accessors using the non-const operator[]
, and then generate a new getter from a const operator[]
if we see one, but this would be suboptimal for structs that have both const & non-const operator[]
– we'd be generating getters only to throw them away later.
So storing these methods in cxxSubscripts
and generating subscript decls after all the members have been imported seems to be the easiest & most efficient solution. WDYT?
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.
Thanks for explaining. This logic makes sense now.
One suggestion about this loop, though. And this is a super small thing: would it make more sense to iterate over cxxSubscripts
rather than all the methods in the record? I think we can do something along the lines of:
for (auto &keyValue : Impl.cxxSubscripts) {
if (keyValue.first.first != result)
continue;
auto getterAndSetter = keyValue.second;
auto subscript = makeSubscript(getterAndSetter.first,
getterAndSetter.second);
result->addMember(subscript);
Impl.cxxSubscripts.erase(keyValue.first);
}
(Maybe with better variable names.)
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.
That looks reasonable to me, fixed it.
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've removed the Impl.cxxSubscripts.erase
statement because is is no longer needed (one cxxSubscripts
entry corresponds to no more than one subscript by definition), and to keep it from invalidating the cxxSubscripts
iterators (since we're now iterating over cxxSubscripts
itself).
@swift-ci please test Windows. |
@swift-ci please test Windows platform. |
a564525
to
01a6ea2
Compare
@swift-ci please smoke test |
@swift-ci please test Windows |
@swift-ci please test Windows platform. |
It's really odd that you can't trigger the Windows bots. @shahmishal any ideas about how to fix this? |
01a6ea2
to
a24f3f3
Compare
This comment has been minimized.
This comment has been minimized.
This mostly looks good to me, other than my remaining comments. Thanks for working on it! As I said early, I would like to find someone else to review the expression synthesis logic. @slavapestov maybe you could review this, or suggest someone who can? |
6ee9a4c
to
231f1b1
Compare
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 looks good to me. Thanks for spending so much time on it. I'll see if I can find someone to review the accessor synthesis part.
231f1b1
to
8b07dcd
Compare
I think I've addressed all the comments, @zoecarver thanks for reviewing this! |
77eaa3e
to
251af4d
Compare
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 looks good to me. Again, thanks for working on this patch. I looked over the accessor synthesis logic and that also looks good to me, but I'd still like another person to look over it.
Please wait to merge this until you get another approval.
@swift-ci please test Windows. |
1 similar comment
@swift-ci please test Windows. |
This change adds support for calling `operator[]` from Swift code via a synthesized Swift subscript. Fixes SR-12598.
251af4d
to
586c675
Compare
@swift-ci please smoke test |
@swift-ci please smoke test macOS platform |
@swift-ci please test Windows. |
@egorzhdan I just looked over this again, and it still looks good to me. I also did a more thorough review of the expression synthesis and it looks good and seems to match the logic elsewhere in the Clang Importer. Let's land this patch! If you still feel good about it, please merge it. |
@zoecarver thanks! I'm still optimistic about this change, so I'm going to merge it now. I'm happy to tweak it later if something doesn't work as expected. |
This change adds support for calling
operator[]
from Swift code via a synthesized Swift subscript.For example:
is translated into
Fixes SR-12598.