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.

diff --git a/src/kernel/activity/ActivityImpl.hpp b/src/kernel/activity/ActivityImpl.hpp
index ed5dc6c..58e146c 100644 (file)
--- 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 7647a18..822ff10 100644 (file)
--- 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 52d0aa7..5e50b88 100644 (file)
--- 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 7945a3a..9d467c7 100644 (file)
--- 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;
 }