Skip to content

[cxx-interop] Add CxxConvertibleToCollection protocol #62243

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
Nov 29, 2022

Conversation

egorzhdan
Copy link
Contributor

Since we don't automatically conform C++ non-random-access collections to Swift.Sequence anymore for performance reasons, we need an alternative way to access the elements of a C++ sequence from Swift.

This allows explicitly converting a C++ sequence to a Swift Array/Set.

Auto-conformance logic will be added in a follow-up patch.

Since we don't automatically conform C++ non-random-access collections to `Swift.Sequence` anymore for performance reasons, we need an alternative way to access the elements of a C++ sequence from Swift.

This allows explicitly converting a C++ sequence to a Swift Array/Set.
@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label Nov 24, 2022
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

1 similar comment
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@ktoso
Copy link
Contributor

ktoso commented Nov 25, 2022

This allows explicitly converting a C++ sequence to a Swift Array/Set.

very cool, finally! :-)

@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test macOS

}
}

extension Array {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be really cool if we could provide optimized implementations for contiguous collections (C++ provides this information in iterator traits). This could just be a reserve + memcpy. I don't know if LLVM is smart enough to do that on its own (probably not).

That can be a follow up patch, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point! I'll do that in a separate patch.

public init<C: CxxConvertibleToCollection>(_ c: C)
where C.RawIterator.Pointee == Element {

self.init()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reserve N elements here so we don't have to potentially allocate each time?

(Can also be follow up. And it would be cool if we had some benchmarks to measure if there's actually an improvement from that change/others.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reserve N elements here so we don't have to potentially allocate each time?

We could do that for C++ RAC, however, C++ RAC are auto-conformed to Swift.RandomAccessCollection which brings an equivalent initializer. For non-random-access collections, we don't really know what N is until we actually iterate over the collection. For some collections iterating twice might produce different results so we can't do that I think.

it would be cool if we had some benchmarks to measure if there's actually an improvement from that change/others

This won't be an improvement unfortunately, we're just making the copy explicit. It's a good idea to add a benchmark though 👍

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.

Thanks!

@egorzhdan egorzhdan merged commit d6f1166 into main Nov 29, 2022
@egorzhdan egorzhdan deleted the egorzhdan/cxx-convertible branch November 29, 2022 12:40
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