remove value_or_abort(), value() now returns ref

Signed-off-by: Teo Koon Peng <[email protected]>

nexus_common/include/nexus_common/error.hpp

@@ -21,7 +21,6 @@
 #include <exception>
 #include <memory>
 #include <optional>
-#include <string>
 #include <type_traits>
 #include <variant>
 
@@ -53,41 +52,31 @@ public: template<typename D,
   Result(D d)
   : _var(std::make_shared<D>(std::move(d))) {}
 
-public: T* value()
+  /**
+   * Returns the value.
+   * @throws std::bad_variant_access if the result is an error.
+   */
+public: constexpr T& value()
   {
-    return std::get_if<T>(&this->_var);
+    return std::get<T>(this->_var);
   }
 
-public: const T* value() const
+  /**
+   * Returns the value.
+   * @throws std::bad_variant_access if the result is an error.
+   */
+public: constexpr const T& value() const
   {
-    return std::get_if<T>(&this->_var);
+    return std::get<T>(this->_var);
   }
 
-public: T* value_or_abort()
-  {
-    if (!this->value())
-    {
-      std::abort();
-    }
-    return this->value();
-  }
-
-public: const T* value_or_abort() const
-  {
-    if (!this->value())
-    {
-      std::abort();
-    }
-    return this->value();
-  }
-
-public: E* error()
+public: constexpr E* error()
   {
     auto* maybe_err = std::get_if<std::shared_ptr<E>>(&this->_var);
     return maybe_err ? maybe_err->get() : nullptr;
   }
 
-public: const E* error() const
+public: constexpr const E* error() const
   {
     auto* maybe_err = std::get_if<std::shared_ptr<E>>(&this->_var);
     return maybe_err ? maybe_err->get() : nullptr;
@@ -100,35 +89,39 @@ template<typename E>
 class Result<void, E>
 {
 public: Result()
-  : _var(std::nullopt) {}
+  : _err(std::nullopt) {}
 
 public: Result(std::shared_ptr<E> e)
-  : _var(std::move(e)) {}
+  : _err(std::move(e)) {}
 
 public: template<typename D,
     typename = std::enable_if_t<std::is_base_of_v<E, D>>>
   Result(D d)
-  : _var(std::make_shared<D>(std::move(d))) {}
+  : _err(std::make_shared<D>(std::move(d))) {}
 
-public: void value_or_abort() const
+  /**
+   * Throws if result is an error.
+   * @throws std::runtime_error if the result is an error.
+   */
+public: constexpr void value() const
   {
-    if (this->error())
+    if (this->_err.has_value())
     {
-      std::abort();
+      throw std::runtime_error("bad value access");
     }
   }
 
-public: E* error()
+public: constexpr E* error()
   {
-    return this->_var.has_value() ? this->_var->get() : nullptr;
+    return this->_err.has_value() ? this->_err->get() : nullptr;
   }
 
-public: const E* error() const
+public: constexpr const E* error() const
   {
-    return this->_var.has_value() ? this->_var->get() : nullptr;
+    return this->_err.has_value() ? this->_err->get() : nullptr;
   }
 
-private: std::optional<std::shared_ptr<E>> _var;
+private: std::optional<std::shared_ptr<E>> _err;
 };
 
 }

nexus_common/src/error_test.cpp

@@ -23,19 +23,19 @@ namespace nexus::common::test {
 
 TEST_CASE("get_result_value_and_error") {
   Result<int> good(1);
-  CHECK(*good.value() == 1);
+  CHECK(good.value() == 1);
   CHECK(!good.error());
 
   const Result<int> const_good(1);
-  CHECK(*const_good.value() == 1);
+  CHECK(const_good.value() == 1);
   CHECK(!const_good.error());
 
   Result<int> bad(std::make_shared<std::runtime_error>("bad"));
-  CHECK(!bad.value());
+  CHECK_THROWS(bad.value());
   CHECK(bad.error());
 
   Result<int> bad_val_constructor(std::runtime_error("bad_val_constructor"));
-  CHECK(!bad_val_constructor.value());
+  CHECK_THROWS(bad_val_constructor.value());
   CHECK(bad_val_constructor.error());
 
   Result<void> void_good;

nexus_workcell_orchestrator/src/job_manager_test.cpp

@@ -91,7 +91,7 @@ TEST_CASE("JobManager") {
 
   SECTION("assign task sets job data correctly") {
     Job* job;
-    job = *job_mgr.assign_task("test").value();
+    job = job_mgr.assign_task("test").value();
     CHECK(!job->bt.has_value());
     CHECK(!job->bt_logging);
     CHECK(job->ctx == nullptr);
@@ -102,15 +102,15 @@ TEST_CASE("JobManager") {
     CHECK(job->tick_count == 0);
 
     SECTION("ticking assigned but unqueued task is a noop") {
-      job_mgr.tick().value_or_abort();
+      job_mgr.tick().value();
       CHECK(job->tick_count == 0);
     }
 
     SECTION("can queue an assigned task") {
       handle_accepted = [&](const JobManager::GoalHandlePtr& goal_handle)
         {
           auto ctx = std::make_shared<Context>(fixture.node);
-          CHECK(*job_mgr.queue_task(goal_handle, ctx,
+          CHECK(job_mgr.queue_task(goal_handle, ctx,
             bt_factory.createTreeFromText(bt)).value() == job);
           CHECK(job->bt.has_value());
           CHECK(job->bt_logging);
@@ -141,7 +141,7 @@ TEST_CASE("JobManager") {
     std::vector<std::string> task_ids{"test1", "test2", "test3"};
     for (const auto& task_id : task_ids)
     {
-      job_mgr.assign_task(task_id).value_or_abort();
+      job_mgr.assign_task(task_id).value();
     }
 
     std::promise<void> done;
@@ -168,11 +168,11 @@ TEST_CASE("JobManager") {
     REQUIRE(done.get_future().wait_for(std::chrono::seconds(
         1)) == std::future_status::ready);
 
-    job_mgr.tick().value_or_abort();
+    job_mgr.tick().value();
     // test1 and test2 should be finished
     CHECK(job_mgr.jobs().size() == 1);
 
-    job_mgr.tick().value_or_abort();
+    job_mgr.tick().value();
     // test3 should now be finished
     CHECK(job_mgr.jobs().size() == 0);
   }

nexus_workcell_orchestrator/src/workcell_orchestrator.cpp

@@ -214,7 +214,7 @@ auto WorkcellOrchestrator::on_activate(
   RCLCPP_INFO(this->get_logger(), "Workcell activated");
   this->_bt_timer = this->create_wall_timer(BT_TICK_RATE, [this]()
       {
-        this->_job_mgr.value().tick().value_or_abort();
+        this->_job_mgr.value().tick().value();
       });
   return CallbackReturn::SUCCESS;
 }
@@ -224,7 +224,7 @@ auto WorkcellOrchestrator::on_deactivate(
 {
   RCLCPP_INFO(this->get_logger(),
     "Halting ongoing task before deactivating workcell");
-  this->_job_mgr->halt_all_jobs().value_or_abort();
+  this->_job_mgr->halt_all_jobs().value();
 
   this->_bt_timer->cancel();
   this->_bt_timer.reset();
@@ -245,7 +245,7 @@ auto WorkcellOrchestrator::on_cleanup(
   this->_signal_wc_srv.reset();
   this->_task_doable_srv.reset();
   this->_cmd_server.reset();
-  this->_job_mgr->halt_all_jobs().value_or_abort();
+  this->_job_mgr->halt_all_jobs().value();
   RCLCPP_INFO(this->get_logger(), "Cleaned up");
   return CallbackReturn::SUCCESS;
 }
@@ -301,7 +301,7 @@ auto WorkcellOrchestrator::_configure(
           "Cancelling specific task is no longer supported, all tasks will be cancelled together to ensure line consistency");
       }
 
-      this->_job_mgr->halt_all_jobs().value_or_abort();
+      this->_job_mgr->halt_all_jobs().value();
       return rclcpp_action::CancelResponse::ACCEPT;
     },
     [this](std::shared_ptr<rclcpp_action::ServerGoalHandle<endpoints::WorkcellRequestAction::ActionType>>
@@ -327,7 +327,7 @@ auto WorkcellOrchestrator::_configure(
         goal_handle->abort(result);
         return;
       }
-      ctx->task = *task_result.value();
+      ctx->task = task_result.value();
 
       auto bt = this->_create_bt(ctx);
       auto job_result =
@@ -744,7 +744,7 @@ void WorkcellOrchestrator::_handle_task_doable(
       resp->success = false;
       return;
     }
-    resp->success = this->_can_perform_task(*r.value());
+    resp->success = this->_can_perform_task(r.value());
     if (resp->success)
     {
       RCLCPP_DEBUG(this->get_logger(), "Workcell can perform task");