Skip to content

Commit

Permalink
Fixes #109 fn overload on task promise::return_void bug (#110)
Browse files Browse the repository at this point in the history 
The return_void() overload was throwing, but this is
incorrect per the standard, it should effectively do
nothing.  This was an error on my part since I over-
loaded the non-void task<T> return_value(T) -> void
and return_value() -> T fns.  This overload isn't
possible with the void versions since the signature
is identical, thus the bug caused the compiler's coroutine
version to re-throw exceptions at the wrong time.
  • Loading branch information
@jbaldwin
jbaldwin authored Dec 4, 2021
1 parent 6bf5e25 commit 5495bfa
Show file tree
Hide file tree
Showing 8 changed files with 56 additions and 50 deletions.
2 changes: 1 addition & 1 deletion inc/coro/generator.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class generator_promise

auto unhandled_exception() -> void { m_exception = std::current_exception(); }

auto return_void() -> void {}
auto return_void() noexcept -> void {}

auto value() const noexcept -> reference_type { return static_cast<reference_type>(*m_value); }

Expand Down
10 changes: 6 additions & 4 deletions inc/coro/sync_wait.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class sync_wait_task_promise : public sync_wait_task_promise_base
return completion_notifier{};
}

auto return_value() -> return_type&&
auto result() -> return_type&&
{
if (m_exception)
{
Expand Down Expand Up @@ -123,7 +123,9 @@ class sync_wait_task_promise<void> : public sync_wait_task_promise_base
return completion_notifier{};
}

auto return_void() -> void
auto return_void() noexcept -> void {}

auto result() -> void
{
if (m_exception)
{
Expand Down Expand Up @@ -169,12 +171,12 @@ class sync_wait_task
if constexpr (std::is_same_v<void, return_type>)
{
// Propagate exceptions.
m_coroutine.promise().return_void();
m_coroutine.promise().result();
return;
}
else
{
return m_coroutine.promise().return_value();
return m_coroutine.promise().result();
}
}

Expand Down
36 changes: 19 additions & 17 deletions inc/coro/task.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ struct promise final : public promise_base

auto return_value(return_type value) -> void { m_return_value = std::move(value); }

auto return_value() const& -> const return_type&
auto result() const& -> const return_type&
{
if (m_exception_ptr)
{
Expand All @@ -80,7 +80,7 @@ struct promise final : public promise_base
return m_return_value;
}

auto return_value() && -> return_type&&
auto result() && -> return_type&&
{
if (m_exception_ptr)
{
Expand All @@ -105,7 +105,9 @@ struct promise<void> : public promise_base

auto get_return_object() noexcept -> task_type;

auto return_void() -> void
auto return_void() noexcept -> void {}

auto result() -> void
{
if (m_exception_ptr)
{
Expand Down Expand Up @@ -143,7 +145,7 @@ class [[nodiscard]] task

explicit task(coroutine_handle handle) : m_coroutine(handle) {}
task(const task&) = delete;
task(task&& other) noexcept : m_coroutine(std::exchange(other.m_coroutine, nullptr)) {}
task(task && other) noexcept : m_coroutine(std::exchange(other.m_coroutine, nullptr)) {}

~task()
{
Expand All @@ -153,9 +155,9 @@ class [[nodiscard]] task
}
}

auto operator=(const task&) -> task& = delete;
auto operator=(const task&)->task& = delete;

auto operator=(task&& other) noexcept -> task&
auto operator=(task&& other) noexcept->task&
{
if (std::addressof(other) != this)
{
Expand All @@ -173,9 +175,9 @@ class [[nodiscard]] task
/**
* @return True if the task is in its final suspend or if the task has been destroyed.
*/
auto is_ready() const noexcept -> bool { return m_coroutine == nullptr || m_coroutine.done(); }
auto is_ready() const noexcept->bool { return m_coroutine == nullptr || m_coroutine.done(); }

auto resume() -> bool
auto resume()->bool
{
if (!m_coroutine.done())
{
Expand All @@ -184,7 +186,7 @@ class [[nodiscard]] task
return !m_coroutine.done();
}

auto destroy() -> bool
auto destroy()->bool
{
if (m_coroutine != nullptr)
{
Expand All @@ -205,12 +207,12 @@ class [[nodiscard]] task
if constexpr (std::is_same_v<void, return_type>)
{
// Propagate uncaught exceptions.
this->m_coroutine.promise().return_void();
this->m_coroutine.promise().result();
return;
}
else
{
return this->m_coroutine.promise().return_value();
return this->m_coroutine.promise().result();
}
}
};
Expand All @@ -227,25 +229,25 @@ class [[nodiscard]] task
if constexpr (std::is_same_v<void, return_type>)
{
// Propagate uncaught exceptions.
this->m_coroutine.promise().return_void();
this->m_coroutine.promise().result();
return;
}
else
{
return std::move(this->m_coroutine.promise()).return_value();
return std::move(this->m_coroutine.promise()).result();
}
}
};

return awaitable{m_coroutine};
}

auto promise() & -> promise_type& { return m_coroutine.promise(); }
auto promise()&->promise_type& { return m_coroutine.promise(); }

auto promise() const& -> const promise_type& { return m_coroutine.promise(); }
auto promise() && -> promise_type&& { return std::move(m_coroutine.promise()); }
auto promise() const&->const promise_type& { return m_coroutine.promise(); }
auto promise()&&->promise_type&& { return std::move(m_coroutine.promise()); }

auto handle() -> coroutine_handle { return m_coroutine; }
auto handle()->coroutine_handle { return m_coroutine; }

private:
coroutine_handle m_coroutine{nullptr};
Expand Down
10 changes: 6 additions & 4 deletions inc/coro/when_all.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,9 @@ class when_all_task_promise<void>

auto unhandled_exception() noexcept -> void { m_exception_ptr = std::current_exception(); }

auto return_void() noexcept -> void
auto return_void() noexcept -> void {}

auto result() -> void
{
if (m_exception_ptr)
{
Expand Down Expand Up @@ -393,7 +395,7 @@ class when_all_task
{
if constexpr (std::is_void_v<return_type>)
{
m_coroutine.promise().return_void();
m_coroutine.promise().result();
return void_value{};
}
else
Expand All @@ -406,7 +408,7 @@ class when_all_task
{
if constexpr (std::is_void_v<return_type>)
{
m_coroutine.promise().return_void();
m_coroutine.promise().result();
return void_value{};
}
else
Expand All @@ -419,7 +421,7 @@ class when_all_task
{
if constexpr (std::is_void_v<return_type>)
{
m_coroutine.promise().return_void();
m_coroutine.promise().result();
return void_value{};
}
else
Expand Down
14 changes: 7 additions & 7 deletions test/test_event.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ TEST_CASE("event single awaiter", "[event]")
REQUIRE_FALSE(task.is_ready());
e.set(); // this will automaticaly resume the task that is awaiting the event.
REQUIRE(task.is_ready());
REQUIRE(task.promise().return_value() == 42);
REQUIRE(task.promise().result() == 42);
}

auto producer(coro::event& event) -> void
Expand All @@ -46,7 +46,7 @@ TEST_CASE("event one watcher", "[event]")

producer(e);

REQUIRE(value.promise().return_value() == 42);
REQUIRE(value.promise().result() == 42);
}

TEST_CASE("event multiple watchers", "[event]")
Expand All @@ -65,9 +65,9 @@ TEST_CASE("event multiple watchers", "[event]")

producer(e);

REQUIRE(value1.promise().return_value() == 42);
REQUIRE(value2.promise().return_value() == 42);
REQUIRE(value3.promise().return_value() == 42);
REQUIRE(value1.promise().result() == 42);
REQUIRE(value2.promise().result() == 42);
REQUIRE(value3.promise().result() == 42);
}

TEST_CASE("event reset", "[event]")
Expand All @@ -82,7 +82,7 @@ TEST_CASE("event reset", "[event]")
REQUIRE_FALSE(value1.is_ready());

producer(e);
REQUIRE(value1.promise().return_value() == 42);
REQUIRE(value1.promise().result() == 42);

e.reset();

Expand All @@ -92,7 +92,7 @@ TEST_CASE("event reset", "[event]")

producer(e);

REQUIRE(value2.promise().return_value() == 42);
REQUIRE(value2.promise().result() == 42);
}

TEST_CASE("event fifo", "[event]")
Expand Down
10 changes: 5 additions & 5 deletions test/test_latch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ TEST_CASE("latch count=0", "[latch]")

task.resume();
REQUIRE(task.is_ready()); // The latch never waits due to zero count.
REQUIRE(task.promise().return_value() == 42);
REQUIRE(task.promise().result() == 42);
}

TEST_CASE("latch count=1", "[latch]")
Expand All @@ -38,7 +38,7 @@ TEST_CASE("latch count=1", "[latch]")

l.count_down();
REQUIRE(task.is_ready());
REQUIRE(task.promise().return_value() == 1);
REQUIRE(task.promise().result() == 1);
}

TEST_CASE("latch count=1 count_down=5", "[latch]")
Expand All @@ -58,7 +58,7 @@ TEST_CASE("latch count=1 count_down=5", "[latch]")

l.count_down(5);
REQUIRE(task.is_ready());
REQUIRE(task.promise().return_value() == 1);
REQUIRE(task.promise().result() == 1);
}

TEST_CASE("latch count=5 count_down=1 x5", "[latch]")
Expand Down Expand Up @@ -86,7 +86,7 @@ TEST_CASE("latch count=5 count_down=1 x5", "[latch]")
REQUIRE_FALSE(task.is_ready());
l.count_down(1);
REQUIRE(task.is_ready());
REQUIRE(task.promise().return_value() == 5);
REQUIRE(task.promise().result() == 5);
}

TEST_CASE("latch count=5 count_down=5", "[latch]")
Expand All @@ -106,5 +106,5 @@ TEST_CASE("latch count=5 count_down=5", "[latch]")

l.count_down(5);
REQUIRE(task.is_ready());
REQUIRE(task.promise().return_value() == 5);
REQUIRE(task.promise().result() == 5);
}
2 changes: 1 addition & 1 deletion test/test_shared_mutex.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ TEST_CASE("mutex many shared and exclusive waiters interleaved", "[shared_mutex]
{
if (st.is_ready())
{
stop = st.promise().return_value();
stop = st.promise().result();
}
}
}
Expand Down
22 changes: 11 additions & 11 deletions test/test_task.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,20 @@ TEST_CASE("task hello world", "[task]")
auto h = []() -> task_type { co_return "Hello"; }();
auto w = []() -> task_type { co_return "World"; }();

REQUIRE(h.promise().return_value().empty());
REQUIRE(w.promise().return_value().empty());
REQUIRE(h.promise().result().empty());
REQUIRE(w.promise().result().empty());

h.resume(); // task suspends immediately
w.resume();

REQUIRE(h.is_ready());
REQUIRE(w.is_ready());

auto w_value = std::move(w).promise().return_value();
auto w_value = std::move(w).promise().result();

REQUIRE(h.promise().return_value() == "Hello");
REQUIRE(h.promise().result() == "Hello");
REQUIRE(w_value == "World");
REQUIRE(w.promise().return_value().empty());
REQUIRE(w.promise().result().empty());
}

TEST_CASE("task void", "[task]")
Expand Down Expand Up @@ -60,7 +60,7 @@ TEST_CASE("task exception thrown", "[task]")
bool thrown{false};
try
{
auto value = task.promise().return_value();
auto value = task.promise().result();
}
catch (const std::exception& e)
{
Expand Down Expand Up @@ -164,7 +164,7 @@ TEST_CASE("task multiple suspends return integer", "[task]")

task.resume(); // third internal suspend
REQUIRE(task.is_ready());
REQUIRE(task.promise().return_value() == 11);
REQUIRE(task.promise().result() == 11);
}

TEST_CASE("task resume from promise to coroutine handles of different types", "[task]")
Expand Down Expand Up @@ -193,7 +193,7 @@ TEST_CASE("task resume from promise to coroutine handles of different types", "[

REQUIRE(task1.is_ready());
REQUIRE(coro_handle1.done());
REQUIRE(task1.promise().return_value() == 42);
REQUIRE(task1.promise().result() == 42);

REQUIRE(task2.is_ready());
REQUIRE(coro_handle2.done());
Expand All @@ -208,7 +208,7 @@ TEST_CASE("task throws void", "[task]")

REQUIRE_NOTHROW(task.resume());
REQUIRE(task.is_ready());
REQUIRE_THROWS_AS(task.promise().return_void(), std::runtime_error);
REQUIRE_THROWS_AS(task.promise().result(), std::runtime_error);
}

TEST_CASE("task throws non-void l-value", "[task]")
Expand All @@ -220,7 +220,7 @@ TEST_CASE("task throws non-void l-value", "[task]")

REQUIRE_NOTHROW(task.resume());
REQUIRE(task.is_ready());
REQUIRE_THROWS_AS(task.promise().return_value(), std::runtime_error);
REQUIRE_THROWS_AS(task.promise().result(), std::runtime_error);
}

TEST_CASE("task throws non-void r-value", "[task]")
Expand All @@ -239,5 +239,5 @@ TEST_CASE("task throws non-void r-value", "[task]")

task.resume();
REQUIRE(task.is_ready());
REQUIRE_THROWS_AS(task.promise().return_value(), std::runtime_error);
REQUIRE_THROWS_AS(task.promise().result(), std::runtime_error);
}

0 comments on commit 5495bfa

Please sign in to comment.