Skip to content
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

Add synchronous generator support for IIterable<T> #1323

Conversation

Fulgen301
Copy link

This implements #1278.

IIterable::First() throws E_CHANGED_STATE if it is called more than one time since the coroutine has already been resumed at that point; however, I am unsure whether this violates the API contract:

If changes are made to the collection, such as adding, modifying, or deleting elements, the iterator is permitted to raise an exception for all future operations.

@Fulgen301
Copy link
Author

@microsoft-github-policy-service agree

Copy link
Contributor

@sylveon sylveon left a comment

Choose a reason for hiding this comment

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

Looks pretty good, you got around to it before me lol

{
suspend_always yield_value(T&& value) noexcept
{
m_result = value;
Copy link
Contributor

Choose a reason for hiding this comment

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

std::move(value)?

@@ -0,0 +1,38 @@
// Intentionally not using pch...
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend adding a test for GetMany(), as well as normally iterating e.g. for (hstring h : Generator())).

What about value types and reference types?

Copy link
Contributor

@sylveon sylveon Jun 20, 2023

Choose a reason for hiding this comment

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

I'm curious what's the behavior regarding AddRef/Release counts. Also, it might be a good idea to add a test checking that not completing the iteration but dropping the IIterable & IIterator will free the coroutine frame (how? throwing an exception in the coroutine? just silently making the coroutine never complete and call destructors? The latter is what happens when you destroy the coroutine frame)

};
}

namespace std
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
namespace std
#ifdef __cpp_lib_coroutine
namespace std
#else
namespace std::experimental
#endif

@kennykerr
Copy link
Collaborator

Unfortunately, a project maintainer is not currently available to review this pull request. Please see the contributing guide for more information. Feel free to keep the conversation going on the related issue.

struct iterable_promise_base : implements<Derived, winrt::Windows::Foundation::Collections::IIterable<TResult>>
{
private:
struct iterator;
Copy link
Member

Choose a reason for hiding this comment

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

indentation looks wrong here. make sure to use 4 spaces to indent

@kennykerr
Copy link
Collaborator

The https://github.com/microsoft/wil library is where we've been collecting implementation helpers such as this. You might want to open an issue on that repo for consideration.

@jonwis
Copy link
Member

jonwis commented Jun 24, 2023

The https://github.com/microsoft/wil library is where we've been collecting implementation helpers such as this. You might want to open an issue on that repo for consideration.

Yes - this looks much more appropriate for wil or https://github.com/microsoft/cpp-async as it doesn't really require cppwinrt on its own. Looking forward to the PR on either of those places!

@sylveon
Copy link
Contributor

sylveon commented Jun 24, 2023

Feels weird to have to IAsyncOperation support be built-in but not IIterable. Perhaps both should be moved to wil?

@kennykerr
Copy link
Collaborator

Async coroutine support is fundamental to C++/WinRT for both consumption and production. Generator support for iteration, while cool, has very narrowly defined application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants