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

Allow methods returning IIterable<T> to be used as synchronous generators #361

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Fulgen301
Copy link

See #349.

This PR adds a synchronous generator implementation for methods that return IIterable<T>.
The generator promise implements both IIterator<T> and IIterable<T> and therefore saves an allocation; however, this can be observed via QueryInterface, so I'm not too sure whether this is a good idea in practice.

@dunhor
Copy link
Member

dunhor commented Sep 19, 2023

IIterable<T> is expected to be multi-pass. Please correct me if I'm wrong, but wouldn't this break if you tried to get more than one IIterator<T> out of it?

@sylveon
Copy link
Contributor

sylveon commented Sep 19, 2023

Yes, but C# already violates that expectation with its own generator methods.

@dunhor dunhor requested a review from jonwis September 19, 2023 19:57
@dunhor
Copy link
Member

dunhor commented Sep 19, 2023

That seems... bad. But I'll defer to @jonwis on this one

@dunhor
Copy link
Member

dunhor commented Sep 19, 2023

I realize that this problem is made worse by the fact that APIs tend to accept an IIterable<T> and rarely an IIterator<T>, even if they don't need multiple passes over the data. My primary concern is that code can pretty easily silently break by new versions of Windows (or other frameworks like WinAppSDK) because an update is made that enumerates more than once. I don't think that's likely to happen often, but it seems easy enough to happen sooner or later.

@jonwis
Copy link
Member

jonwis commented Sep 19, 2023

I realize that this problem is made worse by the fact that APIs tend to accept an IIterable<T> and rarely an IIterator<T>, even if they don't need multiple passes over the data. My primary concern is that code can pretty easily silently break by new versions of Windows (or other frameworks like WinAppSDK) because an update is made that enumerates more than once. I don't think that's likely to happen often, but it seems easy enough to happen sooner or later.

Agree on this; IIterable<T>::First should be returning a fresh new iterator that can be walked over again. It's usually rare, as most implementation code takes the iterable, obtains an iterator, and then pulls values out of that into a vector.

But, it would be a very sharp edge if the IIterable<T> this produces is one-shot.

@sylveon
Copy link
Contributor

sylveon commented Sep 19, 2023

My mistake, the C# iterator methods do support being iterated multiple times, as long as they return IEnumerable<T> (IEnumerator<T> does not, however). C# achieves this by storing the parameters that where passed to the function and rerunning the function from the start when enumerating it a second time, as if you had called the method a second time with the same parameters (which sounds like a potential footgun if any input reference type is modified). C++'s coroutines do not have the required level of introspection to achieve this, they are indeed one-shot.

We could constrain this implementation to IIterator<T> (which is one-shot, as it can only advance forwards and cannot be reset) only but unfortunately this greatly limits its usefulness (can't easily iterate, no API directly taking IIterator<T>).

nit: MoveNext should throw hresult_out_of_bounds if it is called but the state is already Done

`wil::make_iterable_from_iterator` to create an `IIterable<T>` helper object

C++ coroutines can only be evaluated once, which doesn't fulfill
`IIterable<T>`'s requirement that it can return multiple independent iterators.
Thus the generator implementation is constrained to `IIterator<T>`, which may
only be evaluated once, and a helper method is added that returns an
`IIterable<T>` which creates a new generator instance every time an iterator
is requested.
@Fulgen301
Copy link
Author

I've now changed the implementation so that the generator is constrained onto IIterator<T>. To get an IIterable<T>, one needs to use a new helper method, wil::make_iterable_from_iterator, which takes the address of a function returning an IIterator<T> and invokes it for every call to First():

IIterator<winrt::hstring> Generator(int x)
{
	co_yield winrt::to_hstring(x);
}

void Foo()
{
	IIterable<winrt::hstring> iterable = wil::make_iterable_from_iterator(&Generator, 3);
}

template<typename Func, typename... Args, typename TResult = typename details::iterator_result<std::invoke_result_t<Func, Args...>>::type>
winrt::Windows::Foundation::Collections::IIterable<TResult> make_iterable_from_iterator(Func&& func, Args&&... args)
{
return winrt::make<details::iterable_iterator_helper<TResult, Func, Args...>>(std::forward<Func>(func), std::forward<Args>(args)...);
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest adding a way to keep a strong or weak reference to a class here, as well as a pointer. Maybe something similar to C++/WinRT's delegate implementation.

Copy link
Author

Choose a reason for hiding this comment

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

If Func accepts those arguments, it's already possible (e.g. calling a member function with a strong reference that is stored in the IIterable<T> helper with wil::make_iterable_from_iterator(&Foo::Bar, get_strong()). That doesn't work with weak references, but what would the semantics be if the weak pointer's object has been destroyed? Should First return an empty iterator?

Copy link
Contributor

Choose a reason for hiding this comment

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

A member function pointer won't take a strong reference as argument directly via std::invoke/std::apply.

Good point reguarding weak references, lets just ignore that for now.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah looks like std::invoke will try (*arg).(*f)(), I did not know that. Nothing to do here, then.

@dunhor
Copy link
Member

dunhor commented Oct 20, 2023

Mostly looks good to me, however it's been far too long since I've dealt with coroutines in C++. @oldnewthing is probably the most resident coroutine expert if he has any input

@oldnewthing
Copy link
Member

I think this is solving the problem at the wrong level. I think we should add a basic wil::generator to wil/coroutine.h for general-purpose generation, and then the C++/WinRT version can be a wrapper around that.

@sylveon
Copy link
Contributor

sylveon commented Oct 20, 2023

Why not use std::generator then

@Fulgen301
Copy link
Author

Fulgen301 commented Oct 21, 2023

Because it requires C++23, not C++17/20 like the coroutine infrastructure, and has yet to be implemented by the STL.

It also does not provide a way to yield multiple items in sequence without suspending, which I have taken advantage of so that GetMany() produces the requested items without unnecessarily suspending the coroutine just to immediately resume it again. (i have benchmarked this and it improved execution times.)

@Fulgen301
Copy link
Author

Given the one approval, should I still follow the suggestion of adding wil::generator instead?

@oldnewthing
Copy link
Member

I'm not sure what the use case is for returning IIterable<T> instead of a C++ generator. Is this so that you can pass a custom iterator to Windows Runtime methods? Can you give an example of when somebody would actually want this?

@sylveon
Copy link
Contributor

sylveon commented Nov 2, 2023

Either that or implement a WinRT interface which returns IIterable via a generator

@Fulgen301
Copy link
Author

The use cases for this feature are the same ones as in C# - as IIterable<T> is mapped to IEnumerable<T> and IIterator<T> to IEnumerator<T>, it is possible to implement Windows Runtime methods returning IIterable<T> and IIterator<T> using yield statements. This PR adds the same functionality to C++, but as C++ coroutines cannot be reused multiple times - with IEnumerable<T>, you can just ask it for a new enumerator - the actual generator was moved to methods returning IIterator<T>, which cannot be reused to iterate from the beginning anyway. This PR also provides a helper method and a helper object implementing IIterable<T> which creates a new generator every time an IIterator<T> is requested, as most of the Windows Runtime passes around IIterable<T> as opposed to IIterator<T>.

Just like in C#, this can be used any time someone wants to use a generator to implement a collection; right now, the choices are implementing a custom iterator or prefilling and returning a collection of which all elements may not even bee needed, both of which aren't as ergonomic as generators.

@oldnewthing
Copy link
Member

Yes, I understand how this could be used in theory. But are there practical examples of cases in which you want to pass a generator to a Windows Runtime method? I'm hoping to see a compelling scenario like

struct watcher_desires
{
    bool want_adds;
    bool want_removes;
    bool want_updates;
};

auto trigger = DeviceWatcher::GetBackgroundTrigger(make_iterable_from_generator(
    [=](auto desires) -> IIterator<DeviceWatcherEventKind> {
    if (desires.want_adds) co_yield DeviceWatcherEventKind::Add;
    if (desires.want_removes) co_yield DeviceWatcherEventKind::Remove;
    if (desires.want_updates) co_yield DeviceWatcherEventKind::Update;
}, desires));

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.

6 participants