From 93ae3804979dbbefc774e5a70acc32a4407dd7e8 Mon Sep 17 00:00:00 2001
From: Frederic Suter <frederic.suter@cc.in2p3.fr>
Date: Thu, 14 Mar 2019 12:49:02 +0100
Subject: [PATCH] What ActivityImpl child should look like IMHO * ctor does
 nothing more than create a class instance * all fields can be set by
 chainable setters * start() has no parameter

On the S4U side, we should have the same (with CRTP soon)
* ctor calls the Impl ctor and sets thing at user level
* start() does a simcall in which we call all the Impl setters to
  transfer all field values to the kernel and then call Impl::start()

Maybe we should also replace "new *Impl()" in S4U by a call to a
Impl::create() static method to be written.
---
 src/kernel/activity/ActivityImpl.hpp |  1 +
 src/kernel/activity/IoImpl.cpp       | 36 +++++++++++++++++++++-------
 src/kernel/activity/IoImpl.hpp       | 19 +++++++++++----
 src/s4u/s4u_Io.cpp                   | 11 +++++++--
 4 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/src/kernel/activity/ActivityImpl.hpp b/src/kernel/activity/ActivityImpl.hpp
index ed5dc6c0ef..58e146ce94 100644
--- a/src/kernel/activity/ActivityImpl.hpp
+++ b/src/kernel/activity/ActivityImpl.hpp
@@ -31,6 +31,7 @@ public:
 
   const std::string& get_name() const { return name_; }
   const char* get_cname() const { return name_.c_str(); }
+  void set_name(const std::string& name) { name_ = name; }
 
   virtual void suspend();
   virtual void resume();
diff --git a/src/kernel/activity/IoImpl.cpp b/src/kernel/activity/IoImpl.cpp
index 7647a18349..822ff10cea 100644
--- a/src/kernel/activity/IoImpl.cpp
+++ b/src/kernel/activity/IoImpl.cpp
@@ -37,13 +37,6 @@ namespace simgrid {
 namespace kernel {
 namespace activity {
 
-IoImpl::IoImpl(const std::string& name, resource::StorageImpl* storage) : ActivityImpl(name), storage_(storage)
-{
-  this->state_ = SIMIX_RUNNING;
-
-  XBT_DEBUG("Create io impl %p", this);
-}
-
 IoImpl::~IoImpl()
 {
   if (surf_action_ != nullptr)
@@ -51,9 +44,34 @@ IoImpl::~IoImpl()
   XBT_DEBUG("Destroy io %p", this);
 }
 
-IoImpl* IoImpl::start(sg_size_t size, s4u::Io::OpType type)
+IoImplPtr IoImpl::set_name(const std::string& name)
+{
+  ActivityImpl::set_name(name);
+  return this;
+}
+
+IoImplPtr IoImpl::set_type(s4u::Io::OpType type)
+{
+  type_ = type;
+  return this;
+}
+
+IoImplPtr IoImpl::set_size(sg_size_t size)
+{
+  size_ = size;
+  return this;
+}
+
+IoImplPtr IoImpl::set_storage(resource::StorageImpl* storage)
+{
+  storage_ = storage;
+  return this;
+}
+
+IoImpl* IoImpl::start()
 {
-  surf_action_ = storage_->io_start(size, type);
+  state_       = SIMIX_RUNNING;
+  surf_action_ = storage_->io_start(size_, type_);
   surf_action_->set_data(this);
 
   XBT_DEBUG("Create IO synchro %p %s", this, get_cname());
diff --git a/src/kernel/activity/IoImpl.hpp b/src/kernel/activity/IoImpl.hpp
index 52d0aa793b..5e50b88178 100644
--- a/src/kernel/activity/IoImpl.hpp
+++ b/src/kernel/activity/IoImpl.hpp
@@ -15,19 +15,28 @@ namespace kernel {
 namespace activity {
 
 class XBT_PUBLIC IoImpl : public ActivityImpl {
+  resource::StorageImpl* storage_ = nullptr;
+  sg_size_t size_                 = 0;
+  s4u::Io::OpType type_           = s4u::Io::OpType::READ;
+  sg_size_t performed_ioops_      = 0;
+
 public:
   ~IoImpl() override;
-  explicit IoImpl(const std::string& name, resource::StorageImpl* storage);
+  IoImpl() = default;
+
+  IoImplPtr set_name(const std::string& name);
+  IoImplPtr set_size(sg_size_t size);
+  IoImplPtr set_type(s4u::Io::OpType type);
+  IoImplPtr set_storage(resource::StorageImpl* storage);
 
-  IoImpl* start(sg_size_t size, s4u::Io::OpType type);
+  sg_size_t get_performed_ioops() { return performed_ioops_; }
+
+  IoImpl* start();
   void post() override;
   void finish() override;
   void cancel();
   double get_remaining();
-  sg_size_t get_performed_ioops() { return performed_ioops_; }
 
-  resource::StorageImpl* storage_                 = nullptr;
-  sg_size_t performed_ioops_                      = 0;
   static xbt::signal<void(IoImplPtr)> on_start;
   static xbt::signal<void(IoImplPtr)> on_completion;
 };
diff --git a/src/s4u/s4u_Io.cpp b/src/s4u/s4u_Io.cpp
index 7945a3a3c8..9d467c7004 100644
--- a/src/s4u/s4u_Io.cpp
+++ b/src/s4u/s4u_Io.cpp
@@ -16,12 +16,19 @@ namespace s4u {
 Io::Io(sg_storage_t storage, sg_size_t size, OpType type) : Activity(), storage_(storage), size_(size), type_(type)
 {
   Activity::set_remaining(size_);
-  pimpl_ = kernel::activity::IoImplPtr(new kernel::activity::IoImpl(name_, storage_->get_impl()));
+  pimpl_ = kernel::activity::IoImplPtr(new kernel::activity::IoImpl());
 }
 
 Io* Io::start()
 {
-  simix::simcall([this] { boost::static_pointer_cast<kernel::activity::IoImpl>(pimpl_)->start(size_, type_); });
+  simix::simcall([this] {
+    boost::static_pointer_cast<kernel::activity::IoImpl>(pimpl_)
+        ->set_name(name_)
+        ->set_storage(storage_->get_impl())
+        ->set_size(size_)
+        ->set_type(type_)
+        ->start();
+  });
   state_ = State::STARTED;
   return this;
 }
-- 
2.20.1