GorNishanov / coroutines-ts

20 stars 2 forks source link

Propose move construction(or copy elision) for awaitable prvalue for co_await #19

Closed JCYang closed 6 years ago

JCYang commented 7 years ago

In N4680, chapter 5.3.8 subitem 3.4, it is so stated. "—— e is a temporary object copy-initialized from o if o is a prvalue; otherwise e is an lvalue referring to the result of evaluating o." I propose we should allow move-initialized e when o is a prvalue, depends on overload resolution, if we insist we need this temporary e. It is more nature for the expectation of users and for some usage cases where movable-but-not-copyable object involved. I suggest we should also consider support C++17's guarantee copy elision rules for this prvalue which make it more consistent with the rest of the standard. I know the current implementations, both clang and msvc, do copy-elision for co_await prvalue_obj, that's good but we also need a more consistent fix in the TS.

GorNishanov commented 7 years ago

Can you show an example of what this change will enable?

JCYang commented 7 years ago

`namespace coroutine_io{ template<typename AsyncableType, typename... Rs> class completion {//... };

template class completion<AsyncableType&&> { public: using async_task = std::function<void(completion&, AsyncableType&)>; using coro_handle = std::experimental::coroutine_handle; using result_type = boost::system::error_code; using asio_timer = std::unique_ptr; using duration = boost::posix_time::time_duration;

completion(async_task work, AsyncableType&& s) :
    m_async_work(move(work)), m_asio_obj(std::move(s)) {}

completion(completion&& other) = default;

bool await_ready() { return false; }

void await_suspend(coro_handle coro) {
    m_coro = coro;
    m_async_work(*this, m_asio_obj);
}

result_type await_resume() {
    return m_error_code;
}

void done_with_result(boost::system::error_code ec) {
    m_error_code = ec;
    if (m_timeout_timer)
        m_timeout_timer->cancel();
    if (m_coro)
        m_coro.resume();
}

void set_timeout_timer(const duration& timeout) {
    if (timeout.total_milliseconds() > 0) {
        m_timeout_timer = std::make_unique<boost::asio::deadline_timer>(m_asio_obj.get_io_service(), timeout);
        m_timeout_timer->async_wait([this](const boost::system::error_code& ec) {
            if (ec != boost::asio::error::operation_aborted)
                cancel_task();
        });
    }
}

void cancel_task() { 
    m_asio_obj.cancel();
    m_error_code = boost::system::errc::make_error_code(boost::system::errc::timed_out);
    if (m_coro)
        m_coro.resume();
}

private: AsyncableType m_asio_obj; async_task m_async_work; coro_handle m_coro; result_type m_error_code; asio_timer m_timeout_timer; };

template using rw_completion = completion<AsyncableType, std::size_t>;

using tcp_socket = boost::asio::ip::tcp::socket;
using udp_socket = boost::asio::ip::udp::socket;

template<typename AsyncableType, typename EndPoint>
std::enable_if_t<
    (std::is_same_v<std::remove_reference_t<AsyncableType>, tcp_socket> && std::is_same_v<EndPoint, tcp_endpoint>) || 
    (std::is_same_v<std::remove_reference_t<AsyncableType>, udp_socket> && std::is_same_v<EndPoint, udp_endpoint>),
    std::conditional_t<std::is_lvalue_reference_v<AsyncableType>, completion<std::remove_reference_t<AsyncableType>>,
    completion<std::remove_reference_t<AsyncableType>&&>>>
    async_connect(AsyncableType&& ts,
    EndPoint ep, boost::posix_time::time_duration timeout = boost::posix_time::milliseconds(0)) {

    using NonRefAsyncable = std::remove_reference_t<AsyncableType>;

    using completion_type = std::conditional_t<std::is_lvalue_reference_v<AsyncableType>, completion<NonRefAsyncable>,
        completion<NonRefAsyncable&&>>;

    return completion_type([ep, timeout](completion_type& handler, NonRefAsyncable& ts) {
        ts.async_connect(ep, [&handler](const boost::system::error_code& ec) {
            if (ec != boost::asio::error::operation_aborted)
                handler.done_with_result(ec);
        });

        handler.set_timeout_timer(timeout);
    }, std::forward<AsyncableType>(ts));
}

}`

I haven't think about any optimization in the codes yet, but here it is. What I need is a simple interface to extend the current boost::asio class to support coroutine and extend it with an optional timeout(probably a protocol rely on some sort of operation timeout is a bad bad design, but this is not our concerns for the topic) so the users can write all others codes as the boost::asio as they knew, and when need it to work in a coroutine, simply do auto result = co_await coroutine_io::async_connect(the_socket, endpoint); Now it comes two issues. First, how about the user create a temporary socket then co_await for a connection? I will argue whether such usage/design is good, I mean, a temporary socket solely for connection test, but from the viewpoint of a user, auto result = co_await coroutine_io::async_connect(tcp_socket(...), endpoint) should work. So I decide to support this usage, Then I need to move the temp socket into a object that can outlife the call, to store it in the completion<> object returned to co_await operator. the socket class in asio is movable but not copyable. This is the first challenge.

Second, even more serious than the first one. The timeout timer. It is an optional feature in my design, so I would like to create it on demand. without move semantic, it won't work if the implementation follow the TS correctly! Now my code works just because both implementation does not conforms to the TS strictly in this aspect and both get rid of the copy.

I do not think we should treat the prvalue_obj operand of co_await so special, we should just make it more consistent with the rest of the standard and either allow move, or simple do guarantee-copy-elision. That would make the libraries much more flexible.

toby-allsopp commented 7 years ago

Sorry if I'm missing the point here, but doesn't "copy-initialization" already allow move-construction and C++17's "guaranteed copy elision"? I'm looking at http://eel.is/c++draft/dcl.init#15 and http://eel.is/c++draft/dcl.init#17.6.

JCYang commented 7 years ago

toby, you are right! I misunderstood the term copy-initialization. So it is a generalized term. Sorry for not reading the standard correctly.

GorNishanov commented 7 years ago

Shall I close the issue then?

JCYang commented 6 years ago

Yes, please.