jbaldwin/libcoro

The coro::task should not use std::optional

a858438680 opened this issue · 2 comments

Now the coro::task uses the std::optional to store the result, which is incorrect.

The optional is designed to store alterable values, so it requires that the type it stores is copy/move constructible and assignable. However, a normal function can definitely return a value with all these functions deleted.

Let's think about what do we need to return a value from the coro::task. First, the task needs to store the return value inside the promise, and then construct the return value in the caller's frame. So, the requirement of std::optional is too strong and does not meets what we need here. I suggest that we use a custom nullable type to replace this std::optional. A reference implementation is like this

template <typename T>
struct nullable {
public:
    using stored_type = std::remove_cv_t<T>;
    nullable() noexcept: m_engaged(false) {}
    nullable(const nullable&) = delete;
    nullable(nullable&&) = delete;
    nullable& operator=(const nullable&) = delete;
    nullable& operator=(nullable&&) = delete;
    ~nullable() noexcept(std::is_nothrow_destructible_v<stored_type>) {
        if (has_value()) {
            mut_value().~stored_type();
        }
    }

    template <typename... Args>
    void emplace(Args&&... args) noexcept(std::is_nothrow_constructible_v<stored_type, Args...>) {
        new (m_payload) stored_type(static_cast<Args&&>(args)...);
        m_engaged = true;
    }

    bool has_value() const noexcept {
        return m_engaged;
    }

    const stored_type& value() const& noexcept {
        return *std::launder(reinterpret_cast<const stored_type*>(m_payload));
    }

    stored_type&& value() && noexcept {
        return static_cast<stored_type&&>(*std::launder(reinterpret_cast<stored_type*>(m_payload)));
    }
private:
    alignas(stored_type) unsigned char m_payload[sizeof(stored_type)];
    bool m_engaged = false;

    stored_type& mut_value() & noexcept {
        return *std::launder(reinterpret_cast<stored_type*>(m_payload));
    }
};

And by the way, the empty or has_value flag can be combined with whether there is an exception flag. So that the std::exception_ptr can share the storage with the value. I suggest that we use this solution to further reduce the size of task. For example, when return type is uint64_t, std::exception_ptr + uint64_t + bool is acctually 24 bytes (because of padding), but if the std::exception_ptr and uint64_t shares storage, 8 + enum(unsigned char) is 16 bytes.

If you think these changes are good, I will soon create a new pull request which make these changes.

I think we can close this one out? Is there anything else you want to include?

I think we can close this one out? Is there anything else you want to include?

Yes, I think we can close it.