Skip to content

Commit

Permalink
Incorporate feedback from @MiKom
Browse files Browse the repository at this point in the history
- Add license header to connection_evaluator.h
- Use slotInvocation instead of Connection in the ConnectionEvaluator
- Add warnings that deferred connections are experimental
- Improved documentation for deferred connections
  • Loading branch information
LeonMatthesKDAB committed Jan 5, 2024
1 parent 0213588 commit fe953f2
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 22 deletions.
46 changes: 30 additions & 16 deletions src/kdbindings/connection_evaluator.h
Original file line number Diff line number Diff line change
@@ -1,7 +1,16 @@
/*
This file is part of KDBindings.
SPDX-FileCopyrightText: 2023 Klarälvdalens Datakonsult AB, a KDAB Group company <[email protected]>
Author: Shivam Kunwar <[email protected]>
SPDX-License-Identifier: MIT
Contact KDAB at <[email protected]> for commercial licensing options.
*/
#pragma once

#include <functional>
#include <list>
#include <mutex>

#include <kdbindings/connection_handle.h>
Expand All @@ -11,6 +20,8 @@ namespace KDBindings {
/**
* @brief Manages and evaluates deferred Signal connections.
*
* @warning Deferred connections are experimental and may be removed or changed in the future.
*
* The ConnectionEvaluator class is responsible for managing and evaluating connections
* to Signals. It provides mechanisms to delay and control the evaluation of connections.
* It therefore allows controlling when and on which thread slots connected to a Signal are executed.
Expand All @@ -21,16 +32,19 @@ class ConnectionEvaluator
{

public:
/** ConnectionEvaluators are default constructible */
ConnectionEvaluator() = default;

// ConnectionEvaluator is not copyable, as it is designed to manage connections,
/** Connectionevaluators are not copyable */
// As it is designed to manage connections,
// and copying it could lead to unexpected behavior, including duplication of connections and issues
// related to connection lifetimes. Therefore, it is intentionally made non-copyable.
ConnectionEvaluator(const ConnectionEvaluator &) noexcept = delete;

ConnectionEvaluator &operator=(const ConnectionEvaluator &) noexcept = delete;

// ConnectionEvaluators are not moveable, as they are captures by-reference
/** ConnectionEvaluators are not moveable */
// As they are captures by-reference
// by the Signal, so moving them would lead to a dangling reference.
ConnectionEvaluator(ConnectionEvaluator &&other) noexcept = delete;

Expand All @@ -40,43 +54,43 @@ class ConnectionEvaluator
* @brief Evaluate the deferred connections.
*
* This function is responsible for evaluating and executing deferred connections.
* And this function ensures thread safety
* This function is thread safe.
*/
void evaluateDeferredConnections()
{
std::lock_guard<std::mutex> lock(m_connectionsMutex);
std::lock_guard<std::mutex> lock(m_slotInvocationMutex);

for (auto &pair : m_deferredConnections) {
for (auto &pair : m_deferredSlotInvocations) {
pair.second();
}
m_deferredConnections.clear();
m_deferredSlotInvocations.clear();
}

private:
template<typename...>
friend class Signal;

void enqueueSlotInvocation(const ConnectionHandle &handle, const std::function<void()> &connection)
void enqueueSlotInvocation(const ConnectionHandle &handle, const std::function<void()> &slotInvocation)
{
std::lock_guard<std::mutex> lock(m_connectionsMutex);
m_deferredConnections.push_back({ handle, std::move(connection) });
std::lock_guard<std::mutex> lock(m_slotInvocationMutex);
m_deferredSlotInvocations.push_back({ handle, std::move(slotInvocation) });
}

void dequeueSlotInvocation(const ConnectionHandle &handle)
{
std::lock_guard<std::mutex> lock(m_connectionsMutex);
std::lock_guard<std::mutex> lock(m_slotInvocationMutex);

auto handleMatches = [&handle](const auto &invocationPair) {
return invocationPair.first == handle;
};

// Remove all invocations that match the handle
m_deferredConnections.erase(
std::remove_if(m_deferredConnections.begin(), m_deferredConnections.end(), handleMatches),
m_deferredConnections.end());
m_deferredSlotInvocations.erase(
std::remove_if(m_deferredSlotInvocations.begin(), m_deferredSlotInvocations.end(), handleMatches),
m_deferredSlotInvocations.end());
}

std::vector<std::pair<ConnectionHandle, std::function<void()>>> m_deferredConnections;
std::mutex m_connectionsMutex;
std::vector<std::pair<ConnectionHandle, std::function<void()>>> m_deferredSlotInvocations;
std::mutex m_slotInvocationMutex;
};
} // namespace KDBindings
1 change: 1 addition & 0 deletions src/kdbindings/connection_handle.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

#include <kdbindings/genindex_array.h>
#include <kdbindings/utils.h>
#include <memory>

namespace KDBindings {

Expand Down
6 changes: 3 additions & 3 deletions src/kdbindings/genindex_array.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ struct GenerationalIndex {
uint32_t index = 0;
uint32_t generation = 0;

bool operator==(const GenerationalIndex& rhs) const {
// Include both index and generation in the equality check
bool operator==(const GenerationalIndex &rhs) const
{
return index == rhs.index && generation == rhs.generation;
}
};
Expand Down Expand Up @@ -207,6 +207,6 @@ class GenerationalIndexArray
}
};

} //namespace Private
} // namespace Private

} // namespace KDBindings
8 changes: 5 additions & 3 deletions src/kdbindings/signal.h
Original file line number Diff line number Diff line change
Expand Up @@ -263,17 +263,19 @@ class Signal
/**
* @brief Establishes a deferred connection between the provided evaluator and slot.
*
* @warning Deferred connections are experimental and may be removed or changed in the future.
*
* This function allows connecting an evaluator and a slot such that the slot's execution
* is deferred until the conditions evaluated by the `evaluator` are met.
*
* First argument to the function is reference to a shared pointer to the `ConnectionEvaluator` responsible for determining
* First argument to the function is reference to a shared pointer to the ConnectionEvaluator responsible for determining
* when the slot should be executed.
*
* @return An instance of ConnectionHandle, that can be used to disconnect
* or temporarily block the connection.
*
* @note
* The `KDBindings::Signal` class itself is not thread-safe. While the `ConnectionEvaluator` is inherently
* The Signal class itself is not thread-safe. While the ConnectionEvaluator is inherently
* thread-safe, ensure that any concurrent access to this Signal is protected externally to maintain thread safety.
*/
ConnectionHandle connectDeferred(const std::shared_ptr<ConnectionEvaluator> &evaluator, std::function<void(Args...)> const &slot)
Expand Down Expand Up @@ -382,7 +384,7 @@ class Signal
*/
bool blockConnection(const ConnectionHandle &handle, bool blocked)
{
if (m_impl && handle.belongsTo(*this) && handle.m_id.has_value() ) {
if (m_impl && handle.belongsTo(*this) && handle.m_id.has_value()) {
return m_impl->blockConnection(*handle.m_id, blocked);
} else {
throw std::out_of_range("Provided ConnectionHandle does not match any connection\nLikely the connection was deleted before!");
Expand Down

0 comments on commit fe953f2

Please sign in to comment.