Skip to content

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

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

egorzhdan
Copy link
Contributor

@egorzhdan egorzhdan commented Mar 9, 2021

This change adds support for calling operator[] from Swift code via a synthesized Swift subscript.

For example:

struct MyStruct {
  const int &operator[](int x) const { ... }
  int &operator[](int x) { ... }
};

is translated into

struct MyStruct {
  @available(*, unavailable, message: "use subscript")
  mutating func __operatorSubscriptConst(_ x: Int32) -> UnsafePointer<Int32>

  @available(*, unavailable, message: "use subscript")
  mutating func __operatorSubscript(_ x: Int32) -> UnsafeMutablePointer<Int32>

  subscript(x: Int32) -> Int32 {
    mutating get {
      return self.__operatorSubscriptConst(x).pointee
    }
    set {
      self.__operatorSubscript(x).pointee = newValue
    }
  }
}

Fixes SR-12598.

@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan egorzhdan requested a review from zoecarver March 9, 2021 15:41
@gribozavr gribozavr added the c++ interop Feature: Interoperability with C++ label Mar 9, 2021
Copy link
Contributor

@zoecarver zoecarver left a 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.

@@ -3639,6 +3639,29 @@ namespace {
return nullptr;
}
}

for (const auto method : methods) {
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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).

@zoecarver
Copy link
Contributor

@swift-ci please test Windows.

@zoecarver
Copy link
Contributor

@swift-ci please test Windows platform.

@egorzhdan egorzhdan force-pushed the cxx-operator-subscript branch 3 times, most recently from a564525 to 01a6ea2 Compare March 13, 2021 22:51
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan
Copy link
Contributor Author

@swift-ci please test Windows

@zoecarver
Copy link
Contributor

@swift-ci please test Windows platform.

@zoecarver
Copy link
Contributor

It's really odd that you can't trigger the Windows bots. @shahmishal any ideas about how to fix this?

@egorzhdan egorzhdan force-pushed the cxx-operator-subscript branch from 01a6ea2 to a24f3f3 Compare March 14, 2021 12:58
@egorzhdan

This comment has been minimized.

@zoecarver
Copy link
Contributor

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?

@egorzhdan egorzhdan force-pushed the cxx-operator-subscript branch 2 times, most recently from 6ee9a4c to 231f1b1 Compare March 16, 2021 16:29
Copy link
Contributor

@zoecarver zoecarver left a 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.

@egorzhdan egorzhdan force-pushed the cxx-operator-subscript branch from 231f1b1 to 8b07dcd Compare March 17, 2021 19:42
@egorzhdan
Copy link
Contributor Author

I think I've addressed all the comments, @zoecarver thanks for reviewing this!
I might still need your help to run the Windows CI though ;)

@egorzhdan egorzhdan requested a review from zoecarver March 17, 2021 19:45
@egorzhdan egorzhdan force-pushed the cxx-operator-subscript branch 2 times, most recently from 77eaa3e to 251af4d Compare March 18, 2021 20:05
Copy link
Contributor

@zoecarver zoecarver left a 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.

@zoecarver
Copy link
Contributor

@swift-ci please test Windows.

1 similar comment
@zoecarver
Copy link
Contributor

@swift-ci please test Windows.

This change adds support for calling `operator[]` from Swift code via a synthesized Swift subscript.

Fixes SR-12598.
@egorzhdan egorzhdan force-pushed the cxx-operator-subscript branch from 251af4d to 586c675 Compare March 21, 2021 16:27
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test macOS platform

@zoecarver
Copy link
Contributor

@swift-ci please test Windows.

@zoecarver
Copy link
Contributor

zoecarver commented Mar 30, 2021

@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.

@egorzhdan
Copy link
Contributor Author

@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.

@egorzhdan egorzhdan merged commit d0879b9 into swiftlang:main Mar 30, 2021
@kavon kavon mentioned this pull request Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants