From 6a740731c81fb23ebbb5791f61dacb02c751bcc4 Mon Sep 17 00:00:00 2001 From: Frederic Suter Date: Tue, 7 Jan 2020 12:11:56 +0100 Subject: [PATCH] [sonar] don't mix public/private data members --- src/kernel/activity/ConditionVariableImpl.cpp | 5 +--- src/kernel/activity/ConditionVariableImpl.hpp | 23 ++++++++++--------- src/kernel/activity/MutexImpl.hpp | 8 ++++--- src/kernel/activity/SemaphoreImpl.hpp | 5 ++-- src/kernel/activity/SynchroRaw.cpp | 10 ++++---- src/mc/mc_base.cpp | 4 ++-- src/mc/mc_request.cpp | 4 ++-- src/s4u/s4u_ConditionVariable.cpp | 2 +- src/s4u/s4u_Semaphore.cpp | 2 +- src/xbt/xbt_os_synchro.cpp | 4 ++-- 10 files changed, 34 insertions(+), 33 deletions(-) diff --git a/src/kernel/activity/ConditionVariableImpl.cpp b/src/kernel/activity/ConditionVariableImpl.cpp index 976b72e44c..04d034fb1f 100644 --- a/src/kernel/activity/ConditionVariableImpl.cpp +++ b/src/kernel/activity/ConditionVariableImpl.cpp @@ -28,9 +28,6 @@ namespace simgrid { namespace kernel { namespace activity { -ConditionVariableImpl::ConditionVariableImpl() : cond_(this) {} -ConditionVariableImpl::~ConditionVariableImpl() = default; - /** * @brief Signalizes a condition. * @@ -85,7 +82,7 @@ void ConditionVariableImpl::wait(smx_mutex_t mutex, double timeout, actor::Actor /* If there is a mutex unlock it */ if (mutex != nullptr) { - xbt_assert(mutex->owner_ == issuer, + xbt_assert(mutex->get_owner() == issuer, "Actor %s cannot wait on ConditionVariable %p since it does not own the provided mutex %p", issuer->get_cname(), this, mutex); mutex_ = mutex; diff --git a/src/kernel/activity/ConditionVariableImpl.hpp b/src/kernel/activity/ConditionVariableImpl.hpp index 8ad7766894..eb6937b339 100644 --- a/src/kernel/activity/ConditionVariableImpl.hpp +++ b/src/kernel/activity/ConditionVariableImpl.hpp @@ -15,22 +15,23 @@ namespace kernel { namespace activity { class XBT_PUBLIC ConditionVariableImpl { -public: - ConditionVariableImpl(); - ~ConditionVariableImpl(); - - actor::SynchroList sleeping_; /* list of sleeping processes */ MutexImpl* mutex_ = nullptr; - s4u::ConditionVariable cond_; - - void broadcast(); - void signal(); - void wait(MutexImpl* mutex, double timeout, actor::ActorImpl* issuer); + s4u::ConditionVariable piface_; + actor::SynchroList sleeping_; /* list of sleeping actors*/ -private: std::atomic_int_fast32_t refcount_{1}; friend void intrusive_ptr_add_ref(ConditionVariableImpl* cond); friend void intrusive_ptr_release(ConditionVariableImpl* cond); + +public: + ConditionVariableImpl() : piface_(this){}; + ~ConditionVariableImpl() = default; + + void remove_sleeping_actor(actor::ActorImpl& actor) { xbt::intrusive_erase(sleeping_, actor); } + s4u::ConditionVariable* get_iface() { return &piface_; } + void broadcast(); + void signal(); + void wait(MutexImpl* mutex, double timeout, actor::ActorImpl* issuer); }; } // namespace activity } // namespace kernel diff --git a/src/kernel/activity/MutexImpl.hpp b/src/kernel/activity/MutexImpl.hpp index 978fb0e033..ba85ad0f9e 100644 --- a/src/kernel/activity/MutexImpl.hpp +++ b/src/kernel/activity/MutexImpl.hpp @@ -18,6 +18,9 @@ class XBT_PUBLIC MutexImpl { std::atomic_int_fast32_t refcount_{1}; s4u::Mutex piface_; bool locked_ = false; + actor::ActorImpl* owner_ = nullptr; + // List of sleeping actors: + actor::SynchroList sleeping_; public: MutexImpl() : piface_(this) {} @@ -32,9 +35,8 @@ public: MutexImpl* ref(); void unref(); - actor::ActorImpl* owner_ = nullptr; - // List of sleeping actors: - actor::SynchroList sleeping_; + void remove_sleeping_actor(actor::ActorImpl& actor) { xbt::intrusive_erase(sleeping_, actor); } + actor::ActorImpl* get_owner() const { return owner_; } // boost::intrusive_ptr support: friend void intrusive_ptr_add_ref(MutexImpl* mutex) diff --git a/src/kernel/activity/SemaphoreImpl.hpp b/src/kernel/activity/SemaphoreImpl.hpp index b6acb1dd55..7a61019baa 100644 --- a/src/kernel/activity/SemaphoreImpl.hpp +++ b/src/kernel/activity/SemaphoreImpl.hpp @@ -19,10 +19,9 @@ namespace activity { class XBT_PUBLIC SemaphoreImpl { std::atomic_int_fast32_t refcount_{1}; unsigned int value_; - -public: actor::SynchroList sleeping_; /* list of sleeping actors*/ +public: explicit SemaphoreImpl(unsigned int value) : value_(value){}; ~SemaphoreImpl() = default; @@ -32,8 +31,10 @@ public: void acquire(actor::ActorImpl* issuer, double timeout); void release(); bool would_block() { return (value_ == 0); } + void remove_sleeping_actor(actor::ActorImpl& actor) { xbt::intrusive_erase(sleeping_, actor); } unsigned int get_capacity() { return value_; } + bool is_used() { return not sleeping_.empty(); } friend void intrusive_ptr_add_ref(SemaphoreImpl* sem) { diff --git a/src/kernel/activity/SynchroRaw.cpp b/src/kernel/activity/SynchroRaw.cpp index edff83a5b7..6b1aecac0c 100644 --- a/src/kernel/activity/SynchroRaw.cpp +++ b/src/kernel/activity/SynchroRaw.cpp @@ -79,24 +79,24 @@ void RawImpl::finish() switch (simcall->call_) { case SIMCALL_MUTEX_LOCK: - xbt::intrusive_erase(simcall_mutex_lock__get__mutex(simcall)->sleeping_, *simcall->issuer_); + simcall_mutex_lock__get__mutex(simcall)->remove_sleeping_actor(*simcall->issuer_); break; case SIMCALL_COND_WAIT: - xbt::intrusive_erase(simcall_cond_wait__get__cond(simcall)->sleeping_, *simcall->issuer_); + simcall_cond_wait_timeout__get__cond(simcall)->remove_sleeping_actor(*simcall->issuer_); break; case SIMCALL_COND_WAIT_TIMEOUT: - xbt::intrusive_erase(simcall_cond_wait_timeout__get__cond(simcall)->sleeping_, *simcall->issuer_); + simcall_cond_wait_timeout__get__cond(simcall)->remove_sleeping_actor(*simcall->issuer_); simcall_cond_wait_timeout__set__result(simcall, 1); // signal a timeout break; case SIMCALL_SEM_ACQUIRE: - xbt::intrusive_erase(simcall_sem_acquire__get__sem(simcall)->sleeping_, *simcall->issuer_); + simcall_sem_acquire_timeout__get__sem(simcall)->remove_sleeping_actor(*simcall->issuer_); break; case SIMCALL_SEM_ACQUIRE_TIMEOUT: - xbt::intrusive_erase(simcall_sem_acquire_timeout__get__sem(simcall)->sleeping_, *simcall->issuer_); + simcall_sem_acquire_timeout__get__sem(simcall)->remove_sleeping_actor(*simcall->issuer_); simcall_sem_acquire_timeout__set__result(simcall, 1); // signal a timeout break; diff --git a/src/mc/mc_base.cpp b/src/mc/mc_base.cpp index 5612154521..d3b1935d91 100644 --- a/src/mc/mc_base.cpp +++ b/src/mc/mc_base.cpp @@ -118,9 +118,9 @@ bool actor_is_enabled(smx_actor_t actor) case SIMCALL_MUTEX_LOCK: { const kernel::activity::MutexImpl* mutex = simcall_mutex_lock__get__mutex(req); - if (mutex->owner_ == nullptr) + if (mutex->get_owner() == nullptr) return true; - return mutex->owner_->get_pid() == req->issuer_->get_pid(); + return mutex->get_owner()->get_pid() == req->issuer_->get_pid(); } case SIMCALL_SEM_ACQUIRE: { diff --git a/src/mc/mc_request.cpp b/src/mc/mc_request.cpp index 3723ec37f3..3c663e4d2b 100644 --- a/src/mc/mc_request.cpp +++ b/src/mc/mc_request.cpp @@ -325,9 +325,9 @@ std::string simgrid::mc::request_to_string(smx_simcall_t req, int value, simgrid ? simcall_mutex_lock__get__mutex(req) : simcall_mutex_trylock__get__mutex(req))); args = bprintf("locked = %d, owner = %d, sleeping = n/a", mutex.get_buffer()->is_locked(), - mutex.get_buffer()->owner_ != nullptr + mutex.get_buffer()->get_owner() != nullptr ? (int)mc_model_checker->process() - .resolve_actor(simgrid::mc::remote(mutex.get_buffer()->owner_)) + .resolve_actor(simgrid::mc::remote(mutex.get_buffer()->get_owner())) ->get_pid() : -1); break; diff --git a/src/s4u/s4u_ConditionVariable.cpp b/src/s4u/s4u_ConditionVariable.cpp index d55e0eb5a9..c95fc4f6d7 100644 --- a/src/s4u/s4u_ConditionVariable.cpp +++ b/src/s4u/s4u_ConditionVariable.cpp @@ -20,7 +20,7 @@ ConditionVariablePtr ConditionVariable::create() { kernel::activity::ConditionVariableImpl* cond = kernel::actor::simcall([] { return new kernel::activity::ConditionVariableImpl(); }); - return ConditionVariablePtr(&cond->cond_, false); + return ConditionVariablePtr(cond->get_iface(), false); } /** diff --git a/src/s4u/s4u_Semaphore.cpp b/src/s4u/s4u_Semaphore.cpp index 1731d084f1..b4a5ed709c 100644 --- a/src/s4u/s4u_Semaphore.cpp +++ b/src/s4u/s4u_Semaphore.cpp @@ -21,7 +21,7 @@ Semaphore::Semaphore(unsigned int initial_capacity) Semaphore::~Semaphore() { if (sem_ != nullptr) { - xbt_assert(sem_->sleeping_.empty(), "Cannot destroy semaphore since someone is still using it"); + xbt_assert(not sem_->is_used(), "Cannot destroy semaphore since someone is still using it"); delete sem_; } } diff --git a/src/xbt/xbt_os_synchro.cpp b/src/xbt/xbt_os_synchro.cpp index ecfe1343d8..65829c9148 100644 --- a/src/xbt/xbt_os_synchro.cpp +++ b/src/xbt/xbt_os_synchro.cpp @@ -57,12 +57,12 @@ int xbt_cond_timedwait(xbt_cond_t cond, xbt_mutex_t mutex, double delay) void xbt_cond_signal(xbt_cond_t cond) { - cond->cond_.notify_one(); + cond->get_iface()->notify_one(); } void xbt_cond_broadcast(xbt_cond_t cond) { - cond->cond_.notify_all(); + cond->get_iface()->notify_all(); } void xbt_cond_destroy(xbt_cond_t cond) -- 2.20.1