From 0de0208a35620e8fbabcff2b9fe0d886cfe9d0c9 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Fri, 11 Sep 2026 01:24:13 +0200 Subject: [PATCH] qt: make the WebSocket backend and server clang-tidy clean (#514) The three spots morph#514 names, plus the twenty others in the same files that fail the same way -- the issue's actual concern is a consumer running clang-tidy with -warnings-as-errors over vendored morph code, and three of twenty-three does not deliver that. The three from the issue: - The empty `catch (...)` now assigns the "disconnected" fallback itself instead of relying on the initializer above it. bugprone-empty-catch rejects a lexically empty handler however well the intent is commented, and the handler reads better doing the work it describes. - The backoff multiply casts up to double explicitly. `count() * multiplier` already promotes the integral operand, so this is provably the same arithmetic -- verified by static_assert on both the resulting type and the value across a range of inputs -- with the one deliberate narrowing left on the outside where the existing cast documents it. - The QObject-parented `new QTimer(this)` gets a per-site NOLINT, not the directory-wide suppression the issue suggested. Measured first: this is the only such site in src/, so disabling cppcoreguidelines-owning-memory for the directory would turn a real check off to silence one line. If QObject-parented `new` becomes common here, tests/.clang-tidy is the precedent for doing it properly. The rest are mechanical: designated initializers, const correctness, explicit null comparisons, parentheses around the token-bucket refill (correct as written -- the check only wants them stated), consumeToken made const, and deleted copy/move on two classes that were never copyable in practice. All 1689 ctest tests pass under clang and gcc with Qt enabled; the 67 Qt tests specifically pass. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- include/morph/qt/qt_websocket_backend.hpp | 10 ++++- include/morph/qt/qt_websocket_server.hpp | 13 ++++-- src/qt/qt_websocket_backend.cpp | 51 +++++++++++++++-------- src/qt/qt_websocket_server.cpp | 29 ++++++++----- 4 files changed, 71 insertions(+), 32 deletions(-) diff --git a/include/morph/qt/qt_websocket_backend.hpp b/include/morph/qt/qt_websocket_backend.hpp index a940a5fd..780ce42d 100644 --- a/include/morph/qt/qt_websocket_backend.hpp +++ b/include/morph/qt/qt_websocket_backend.hpp @@ -159,6 +159,14 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// @brief Closes the socket and cleans up pending operations. ~QtWebSocketBackend() override; + // Neither copyable nor movable: the backend owns a QWebSocket bound to this + // object's address through Qt's signal/slot connections, and its pending-call + // maps are keyed to callbacks that capture `this`. + QtWebSocketBackend(const QtWebSocketBackend&) = delete; + QtWebSocketBackend& operator=(const QtWebSocketBackend&) = delete; + QtWebSocketBackend(QtWebSocketBackend&&) = delete; + QtWebSocketBackend& operator=(QtWebSocketBackend&&) = delete; + /// @brief Pumps the Qt event loop until the socket is connected or @p timeoutMs elapses. /// /// Must be called on the Qt event loop thread after construction. @@ -498,7 +506,7 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { struct PendingExecute { std::shared_ptr<::morph::async::detail::CompletionState>> state; std::function(std::string_view)> deserialize; - ::morph::exec::IExecutor* cbExec; + ::morph::exec::IExecutor* cbExec{nullptr}; }; uint64_t _nextCallId{0}; std::unordered_map _pending; diff --git a/include/morph/qt/qt_websocket_server.hpp b/include/morph/qt/qt_websocket_server.hpp index bbee9fd7..6edb2c3f 100644 --- a/include/morph/qt/qt_websocket_server.hpp +++ b/include/morph/qt/qt_websocket_server.hpp @@ -135,6 +135,13 @@ class QtWebSocketServer : public QObject { /// @brief Closes the server and disconnects all clients. ~QtWebSocketServer() override; + // Neither copyable nor movable: a QObject with signal/slot connections bound + // to this address, holding per-client state keyed by QWebSocket pointer. + QtWebSocketServer(const QtWebSocketServer&) = delete; + QtWebSocketServer& operator=(const QtWebSocketServer&) = delete; + QtWebSocketServer(QtWebSocketServer&&) = delete; + QtWebSocketServer& operator=(QtWebSocketServer&&) = delete; + /// @brief Starts listening for incoming WebSocket connections. /// /// Refuses — returns `false` without binding, and logs at @@ -202,10 +209,10 @@ class QtWebSocketServer : public QObject { double tokens = 0.0; /// @brief Last time `tokens` was refilled (used to compute elapsed time on the next frame). - std::chrono::steady_clock::time_point lastRefill{}; + std::chrono::steady_clock::time_point lastRefill; /// @brief Last time any frame was received on this connection (drives `idleTimeout`). - std::chrono::steady_clock::time_point lastActivity{}; + std::chrono::steady_clock::time_point lastActivity; /// @brief One-shot timer enforcing `handshakeTimeout`; `nullptr` once cancelled by the /// first frame, or if `handshakeTimeout == 0`. @@ -222,7 +229,7 @@ class QtWebSocketServer : public QObject { /// @param state Per-connection state to update. /// @return `true` if a token was available (the frame is admitted), `false` if /// the bucket was empty (the frame must be dropped). - bool consumeToken(ClientState& state); + bool consumeToken(ClientState& state) const; /// @brief Qt slot: periodic sweep that closes any connection idle past `idleTimeout`. Q_SLOT void onHousekeepingTick(); diff --git a/src/qt/qt_websocket_backend.cpp b/src/qt/qt_websocket_backend.cpp index 0eab82ad..d24bad8b 100644 --- a/src/qt/qt_websocket_backend.cpp +++ b/src/qt/qt_websocket_backend.cpp @@ -179,8 +179,10 @@ bool QtWebSocketBackend::registerModelAsync( // happens, cancelPending() drains this queue too and still invokes // onError exactly once. std::scoped_lock const lock{_pendingMtx}; - _queuedRegistrations.push_back( - QueuedRegistration{typeId, std::string{contextKey}, std::move(onRegistered), std::move(onError)}); + _queuedRegistrations.push_back(QueuedRegistration{.typeId = typeId, + .contextKey = std::string{contextKey}, + .onRegistered = std::move(onRegistered), + .onError = std::move(onError)}); return true; } sendRegisterAsync(typeId, contextKey, std::move(onRegistered), std::move(onError)); @@ -204,7 +206,8 @@ void QtWebSocketBackend::sendRegisterAsync(const std::string& typeId, std::strin auto const encoded = QString::fromStdString(::morph::wire::encode(env)); { std::scoped_lock const lock{_pendingMtx}; - _pendingRegistrations[callId] = PendingRegistration{std::move(onRegistered), std::move(onError)}; + _pendingRegistrations[callId] = + PendingRegistration{.onRegistered = std::move(onRegistered), .onError = std::move(onError)}; } _socket.sendTextMessage(encoded); } @@ -391,7 +394,8 @@ bool QtWebSocketBackend::assignPrimaryAsync(::morph::exec::detail::ModelId mid, auto const encoded = QString::fromStdString(::morph::wire::encode(env)); { std::scoped_lock const lock{_pendingMtx}; - _pendingAssigns[callId] = PendingAssign{std::move(onRegistered), std::move(onError)}; + _pendingAssigns[callId] = + PendingAssign{.onRegistered = std::move(onRegistered), .onError = std::move(onError)}; } _socket.sendTextMessage(encoded); return true; @@ -446,7 +450,7 @@ ::morph::async::Completion> QtWebSocketBackend::execute( return comp; } - uint64_t callId = ++_nextCallId; + uint64_t const callId = ++_nextCallId; ::morph::wire::Envelope env; env.kind = "execute"; env.callId = callId; @@ -457,8 +461,9 @@ ::morph::async::Completion> QtWebSocketBackend::execute( env.session = std::move(call.session); { - std::scoped_lock lock{_pendingMtx}; - _pending[callId] = PendingExecute{compState, std::move(call.deserializeResult), cbExec}; + std::scoped_lock const lock{_pendingMtx}; + _pending[callId] = + PendingExecute{.state = compState, .deserialize = std::move(call.deserializeResult), .cbExec = cbExec}; } _socket.sendTextMessage(QString::fromStdString(::morph::wire::encode(env))); @@ -471,7 +476,7 @@ void QtWebSocketBackend::cancelPending(const std::exception_ptr& exc) { std::vector drainedQueue; std::unordered_map drainedAssigns; { - std::scoped_lock lock{_pendingMtx}; + std::scoped_lock const lock{_pendingMtx}; drainedExecutes.swap(_pending); drainedRegistrations.swap(_pendingRegistrations); drainedQueue.swap(_queuedRegistrations); @@ -480,20 +485,27 @@ void QtWebSocketBackend::cancelPending(const std::exception_ptr& exc) { // just drop the bookkeeping, there is no callback to invoke. _pendingDeregisters.clear(); } - for (auto& [_, pending] : drainedExecutes) { + for (auto& [ignoredCallId, pending] : drainedExecutes) { if (pending.state) { pending.state->setException(exc); } } - std::string message = "disconnected"; + // Every caller passes a `make_exception_ptr`, so `rethrow_exception` always + // throws and one of the two handlers always assigns `message`. + std::string message; try { std::rethrow_exception(exc); } catch (const std::exception& concrete) { message = concrete.what(); } catch (...) { - // Non-std::exception thrown in: keep the "disconnected" fallback above. - } - for (auto& [_, pending] : drainedRegistrations) { + // A non-std::exception carries no portable message, so report the + // disconnect itself -- which is what a caller can act on anyway. Doing + // it here rather than as an initializer above keeps the handler from + // being lexically empty, which `bugprone-empty-catch` rejects however + // well the intent is commented (morph#514). + message = "disconnected"; + } + for (auto& [ignoredCallId, pending] : drainedRegistrations) { if (pending.onError) { pending.onError(message); } @@ -509,7 +521,7 @@ void QtWebSocketBackend::cancelPending(const std::exception_ptr& exc) { entry.onError(message); } } - for (auto& [_, pending] : drainedAssigns) { + for (auto& [ignoredCallId, pending] : drainedAssigns) { if (pending.onError) { pending.onError(message); } @@ -527,8 +539,13 @@ void QtWebSocketBackend::setSession(::morph::session::Context session) { _sessio void QtWebSocketBackend::scheduleReconnect() { _reconnectTimer.start(static_cast(_currentReconnectDelay.count())); // Pre-compute the next backoff so the timer above used the *current* one. - auto next = std::chrono::milliseconds{ - static_cast(_currentReconnectDelay.count() * _cfg.backoffMultiplier)}; + // Cast up to double first so the multiplication is openly floating-point. + // Written as `count() * backoffMultiplier` the integral `rep` is narrowed to + // double *inside* the expression, which the narrowing-conversions checks + // flag separately from the explicit cast back (morph#514). Same arithmetic, + // same result -- only the one deliberate narrowing is left, on the outside. + auto next = std::chrono::milliseconds{static_cast( + static_cast(_currentReconnectDelay.count()) * _cfg.backoffMultiplier)}; _currentReconnectDelay = std::min(next, _cfg.maxReconnectDelay); } @@ -694,7 +711,7 @@ void QtWebSocketBackend::onTextMessage(const QString& message) { } _pendingReply = std::move(msg); - if (_syncLoop) { + if (_syncLoop != nullptr) { _syncLoop->quit(); } } diff --git a/src/qt/qt_websocket_server.cpp b/src/qt/qt_websocket_server.cpp index ff3b23f5..61ce9e55 100644 --- a/src/qt/qt_websocket_server.cpp +++ b/src/qt/qt_websocket_server.cpp @@ -23,7 +23,7 @@ QtWebSocketServer::QtWebSocketServer(::morph::backend::RemoteServer& server, qui : QObject{parent}, _server{server}, _requestedPort{port}, - _cfg{cfg}, + _cfg{std::move(cfg)}, #ifndef QT_NO_SSL _wsServer{QStringLiteral("morph"), tls.has_value() ? QWebSocketServer::SecureMode : QWebSocketServer::NonSecureMode, this}, @@ -73,7 +73,7 @@ void QtWebSocketServer::close() { for (auto& [socket, state] : _clients) { socket->disconnect(this); _server.closeConnection(state.cid); - if (state.handshakeTimer) { + if (state.handshakeTimer != nullptr) { state.handshakeTimer->stop(); state.handshakeTimer->deleteLater(); } @@ -149,7 +149,7 @@ bool QtWebSocketServer::closeGracefully(std::chrono::milliseconds deadline) { void QtWebSocketServer::onNewConnection() { QWebSocket* socket = _wsServer.nextPendingConnection(); - if (!socket) { + if (socket == nullptr) { return; } if (_cfg.maxConnections != 0 && _clients.size() >= _cfg.maxConnections) { @@ -176,6 +176,13 @@ void QtWebSocketServer::onNewConnection() { ::morph::log::logInfo("[QtWebSocketServer] connection {} accepted ({} live)", state.cid, _clients.size() + 1); if (_cfg.handshakeTimeout.count() > 0) { + // Qt parent-child ownership: `this` owns the timer and deletes it, and every path that + // drops the reference calls deleteLater() first (see the handshakeTimer handling below). + // cppcoreguidelines-owning-memory has no model of that convention -- it flags any raw + // `new` bound to a non-gsl::owner pointer (morph#514). Suppressed here rather than for the + // whole directory: this is the only such site in src/, and a directory-wide disable would + // turn the check off for a future `new` that really is unowned. + // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) — QObject parent owns this auto* timer = new QTimer(this); timer->setSingleShot(true); connect(timer, &QTimer::timeout, this, [this, socket] { @@ -190,15 +197,15 @@ void QtWebSocketServer::onNewConnection() { _clients.emplace(socket, state); } -bool QtWebSocketServer::consumeToken(ClientState& state) { +bool QtWebSocketServer::consumeToken(ClientState& state) const { if (_cfg.messagesPerSecond == 0) { return true; // unbounded (today's behavior) } auto const now = std::chrono::steady_clock::now(); double const elapsedSeconds = std::chrono::duration(now - state.lastRefill).count(); state.lastRefill = now; - double const capacity = static_cast(_cfg.messagesPerSecond); - state.tokens = std::min(capacity, state.tokens + elapsedSeconds * capacity); + auto const capacity = static_cast(_cfg.messagesPerSecond); + state.tokens = std::min(capacity, state.tokens + (elapsedSeconds * capacity)); if (state.tokens < 1.0) { return false; } @@ -208,7 +215,7 @@ bool QtWebSocketServer::consumeToken(ClientState& state) { void QtWebSocketServer::onTextMessage(const QString& message) { auto* socket = qobject_cast(sender()); - if (!socket) { + if (socket == nullptr) { return; } auto iter = _clients.find(socket); @@ -217,7 +224,7 @@ void QtWebSocketServer::onTextMessage(const QString& message) { } ClientState& state = iter->second; state.lastActivity = std::chrono::steady_clock::now(); - if (state.handshakeTimer) { + if (state.handshakeTimer != nullptr) { state.handshakeTimer->stop(); state.handshakeTimer->deleteLater(); state.handshakeTimer = nullptr; @@ -259,7 +266,7 @@ void QtWebSocketServer::onTextMessage(const QString& message) { return; } - QPointer weakSocket{socket}; + QPointer const weakSocket{socket}; _server.handle( message.toStdString(), [weakSocket](const std::string& reply) { @@ -277,7 +284,7 @@ void QtWebSocketServer::onTextMessage(const QString& message) { void QtWebSocketServer::onDisconnected() { auto* socket = qobject_cast(sender()); - if (!socket) { + if (socket == nullptr) { return; } auto iter = _clients.find(socket); @@ -290,7 +297,7 @@ void QtWebSocketServer::onDisconnected() { iter->second.cid, _clients.size() - 1, static_cast(socket->closeCode()), socket->closeReason().toStdString()); _server.closeConnection(iter->second.cid); - if (iter->second.handshakeTimer) { + if (iter->second.handshakeTimer != nullptr) { iter->second.handshakeTimer->stop(); iter->second.handshakeTimer->deleteLater(); }