Skip to content

qt: make the WebSocket backend and server clang-tidy clean (#514) - #516

Merged
Yaraslaut merged 1 commit into
masterfrom
qt-clang-tidy-514
Sep 11, 2026
Merged

qt: make the WebSocket backend and server clang-tidy clean (#514)#516
Yaraslaut merged 1 commit into
masterfrom
qt-clang-tidy-514

Conversation

@Yaraslaut

Copy link
Copy Markdown
Member

Closes #514.

Scope

The issue names three spots. I fixed those and the twenty others in the same two files that fail the same way — the stated concern is "a consumer that also runs clang-tidy with this check as an error gets a build failure on unmodified vendored code", and 3 of 23 doesn't deliver that. The Qt WebSocket backend and server, headers included, now produce zero clang-tidy findings.

The three from the issue

1. bugprone-empty-catch. The handler now assigns the "disconnected" fallback itself rather than relying on the initializer above it:

std::string message;
try { std::rethrow_exception(exc); }
catch (const std::exception& concrete) { message = concrete.what(); }
catch (...) { message = "disconnected"; }

Every caller passes a make_exception_ptr, so rethrow_exception always throws and one handler always assigns. Behaviour identical, and the handler now does the work its comment describes instead of being lexically empty — which the check rejects however well commented.

2. Narrowing in the backoff. Cast up to double explicitly so the multiplication is openly floating-point, leaving the one deliberate narrowing on the outside where the existing cast documents it.

Provably the same arithmetic — rep * double already promotes the integral operand, so this only names the promotion. Verified rather than asserted:

static_assert(std::is_same_v<decltype(rep{7} * 1.7), decltype(static_cast<double>(rep{7}) * 1.7)>);
constexpr bool same(rep n, double m) { return static_cast<rep>(n * m) == static_cast<rep>(static_cast<double>(n) * m); }
static_assert(same(1, 1.5) && same(250, 1.7) && same(999983, 2.3) && same(0, 3.0) && same(7, 0.5));

3. cppcoreguidelines-owning-memory on the QObject-parented new. I did not take the project-wide suppression the issue suggested, and want to flag the disagreement rather than bury it.

I measured first: new QTimer(this) is the only such site in all of src/. (There are three more repo-wide — examples/common/testkit, examples/common/wasm_spike, examples/kanban/gui_lib — but they're in three different directories, so no single directory config would have covered them either, and examples aren't what a consumer vendors.) Disabling the check for src/qt/ would turn off a genuinely useful check across the directory to silence one line, and would silently accept a future new that really is unowned. So: a per-site NOLINT carrying the Qt-ownership rationale. If QObject-parented new becomes common in src/, tests/.clang-tidy is the precedent for doing it directory-wide properly, and I'd switch then.

The other twenty

Mechanical, no behaviour change: designated initializers (4), const correctness (5), explicit != nullptr instead of implicit pointer-to-bool (7), consumeToken made const, auto on a cast initializer, structured-binding names above the identifier-length floor (3), a redundant member init pair, and deleted copy/move on two classes that were never copyable in practice (both hold Qt signal/slot connections bound to their own address).

One worth a second look was readability-math-missing-parentheses on state.tokens + elapsedSeconds * capacity — that is correct token-bucket refill math, and the check only wants the precedence stated. Parenthesised, not changed.

Verification

  • clang-tidy: 0 findings across src/qt/qt_websocket_backend.cpp, src/qt/qt_websocket_server.cpp, include/morph/qt/qt_websocket_backend.hpp, include/morph/qt/qt_websocket_server.hpp (was 23 + 5 in the headers)
  • clang and gcc, both with MORPH_BUILD_QT=ON: clean
  • 1689/1689 ctest tests pass, including all 67 Qt tests and both WebSocket interop cases
  • tree-wide clang-format --dry-run -Werror, spec-citation lint: clean

🤖 Generated with Claude Code

https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Yaraslaut
Yaraslaut merged commit 4017228 into master Sep 11, 2026
49 checks passed
@Yaraslaut
Yaraslaut deleted the qt-clang-tidy-514 branch September 11, 2026 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

qt: two spots in qt_websocket_backend.cpp trip -warnings-as-errors clang-tidy checks

1 participant