On Tue, Sep 18, 2012 at 10:45 PM, Eugene Yakubovich <eyakubovich@gmail.com>wrote:
On Mon, Sep 17, 2012 at 11:24 AM, Giovanni Piero Deretta
I like the idea that has been proposed during the review that the result of a coroutine should be retrivable at any time via a get method. I dislike the proposed optional<T> result for operator(). If the previous delayed result functionality is implemented, operator() should just return *this, so that the subsequent get() call can be chained or the result tested for non empty. This change has the potential of deeply affecting the interface, so I would really like to see it subject to a mini review before acceptance.
In this case, get() or operator* (like in your sketch impl) should return T&, not T. Otherwise you get a surprise move: T x = *c; // OK, value has been moved into x. T y = *c; // y contains an empty value
Yes, definitely, get() should return a reference. BTW, I initially thought that operator* and operator++ would work well for the coroutine interface, to mimic the iterator interface, but I think it was a mistake. An explicit get() is better.
It has been suggested that coroutine::self be removed an have a thread specific object. I strongly disagree with this option. First of all it
will
make it too easy to break type safety:
typedef this_coroutine<int(int)> this_coro;
typedef this_coroutine<void()> that_coro;
coroutine<int(int)>([]{ that_coro::yield(); // oops compiles fine, UB at runtime. });
This is a good point. I now think I'd prefer type safety here so I agree that using a thread specific object is bad.
Help me convince Oliver then ;).
This means that the parameter no longer represent the coroutine itself, but, more logically, the caller coroutine. This makes very obvious the analogy between coroutines and channels. It also blurs the division
between
symmetric and asymmetric coroutines. Bonus points if you had a function that infers the coroutine singnature from the coroutine functor operator() (I like to call this function callcc for obvious reasons [ http://en.wikipedia.org/wiki/Call-with-current-continuation]):
auto my_coroutine = callcc([](coroutine<void(int)> caller) { ... } );
I agree that there're similarities with call/cc but I'm not sure it's
exactly the same. call/cc returns the "yielded" value, not a coroutine. It only returns a coroutine if the "yield" was bundled with another call to call/cc. So to avoid the confusion, a different name might be best.
Also technically what it captures is not a full continuation but a one shot continuation so the name is misleading; the literature uses callcc1 for them. The name is a bit obscure and it was an half joke anyway.
I do not like the separation between coroutines and generators. Instead the generator class should be dropped. The reason that I had generator as a separate class in the original coroutine SoC is because I wanted to give it iterator semantics (this also required internally using a reference counted pointer to make the iterator copiable, which I disliked). Instead I propose to make every coroutine<T()> fulfill the the input range concept and every coroutine<void(T)> fulfill the output range concept (this is a functionality not covered by generators).
While I like this suggestion, I worry that having one template specialization model one concept and another specialization model another concept (Input and Output iterators) might be too confusing.
It is not unlike an std::function<void(T)> modelling an UnaryFunction and an std::function<T()> modelling a Generator.
The last suggestion works very well if coupled with the suggestion of making 'self' a full coroutine:
std::vector<int> a = { ... }; std::vector<int> b = { ... };
std::cout << "The result of 'merge(a,b) is:\n"; for(auto x : callcc([&](coroutine<void(int)> c) { std::merge(a.begin(), a.end(), b.begin(), b.end(), c.begin()); }) { std::cout << x << "\n"; }
This does look compelling.
Also, the sketch implementation's continuation class is copyable.
the continuation in my sketch is movable only: 97: continuation(const continuation&) = delete;
It would also be great if the coroutine function could return void instead of making the user return (moved) coroutine. I understand why it's needed from the implementation point of view but I wonder if there's a way around it.
I thought about it, but I couldn't find any way around. As a plus it allows a few tricks like exiting to a different continuation than the caller. Anyways, I think it becomes natural after a while. Also note that you do not really need the explicit move on return in c++11. -- GPD