Repository navigation
chore(build): clear the GCC 14 deprecation and nodiscard warnings in the dialog and tests. Principle VIII. - #6366
Conversation
…the dialog and tests. Principle VIII. A clean GCC 14 build reported four project-owned warning groups that have a direct fix: - RadioSetupDialog's hardware-settings reply lambda used `[=]`, which implicitly captures `this` (deprecated in C++20). It is now `[=, this]`, the same capture. - Six tests discarded a [[nodiscard]] result (QFile::open, QTemporaryFile::open, QTest::qWaitForWindowExposed), so a failed open or a window that never exposed went unnoticed. Each now asserts it through the test's own harness (QVERIFY, check or report). - Two tests built QDragEnterEvent from a QPoint, which Qt deprecates in favour of the QPointF constructor. - unified_title_bar_test called QApplication::setActiveWindow directly; it now uses activateForTest(), which already wraps it for the suite. No production behaviour changes. Verified on Linux (GCC 14, Qt 6.12): the app and the six touched test targets build with none of these warnings, and all six tests pass under QT_QPA_PLATFORM=offscreen. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…t::activateWindow. Principle VIII. The previous commit routed this call through activateForTest(), which falls back to the deprecated QApplication::setActiveWindow when the platform declines to activate. Qt's own replacement for setActiveWindow is QWidget::activateWindow, and the test passes under the offscreen platform with it, so the helper and its include are not needed here. Verified on Linux (GCC 14.2, Qt 6.12.0): the target builds with no warnings and unified_title_bar_test passes under QT_QPA_PLATFORM=offscreen. Refs aethersdr#6367 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
Partially. #6367 enumerates seven files and exact line numbers, and every one of them is touched here — nothing is missing and nothing extra arrives. Six of the seven edits do what the issue asks. The three settings_browser_dialog_test.cpp sites do not: they silence the [[nodiscard]] warning without ever being able to fail the test, which is the opposite of the issue's actual ask ("tests that fail when a precondition they rely on fails"). Details in Blockers.
Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/gui/RadioSetupDialog.cpp:4337 |
[=] → [=, this] |
Yes, #6367 names this line | In scope. Semantically identical — this was already captured (members used at 4362, 4375-4376) |
tests/qrz_callsign_test.cpp:189 |
f.open() → check(f.open(), …) |
Yes | In scope, correct — check() increments failures (line 36) and main returns it (304) |
tests/system_info_dialog_test.cpp:431,465 |
open() → report("…", open()) |
Yes | In scope, correct — report() increments g_failures (line 53), main returns it (883) |
tests/settings_browser_dialog_test.cpp:245,290,355 |
ignored return → QVERIFY(…) |
Yes | In scope, but the mechanism does not work — see Blocker 1 |
tests/stage_tab_bar_drag_test.cpp:70, tests/workspace_controller_test.cpp:93 |
QDragEnterEvent QPoint ctor → QPointF |
Yes | In scope. Legitimate at our floor — see below |
tests/unified_title_bar_test.cpp:982 |
setActiveWindow → activateWindow() |
Yes | In scope. Qt's own prescribed replacement; in-tree precedent confirmed |
Everything in the diff is explained by #6367. No new public surface, no settings keys, no capability fields, no CHANGELOG.md entry, no removed guard. No socket test is added, removed, or modified: QTcpServer/QTcpSocket/QUdpSocket/QLocalServer/QWebSocketServer/bind/listen/connectToHost all return zero hits across the six touched test files.
Two merge-order notes, neither blocking:
- #6359 (same author, open) also edits
tests/unified_title_bar_test.cpp, on the Alt+F block at ~1007 rather than line 982. Its body explicitly lists "setActiveWindowat line 983" as a warning it left alone, so the two are complementary, but they land in the same ~30-line region and whichever merges second will need a rebase. workspace_controller_test.cpp:93now passes the fractionalposto the enter event where it previously rounded via.toPoint(). Sub-pixel, and theQDropEventon line 96 already used the unrounded value, so this makes the pair agree rather than diverge.
Blockers
1. tests/settings_browser_dialog_test.cpp:245, :290, :355 — QVERIFY cannot fail this test, and on failure it silently skips the rest of the case.
QVERIFY(QTest::qWaitForWindowExposed(&dlg));This file is a hand-rolled harness: int main() at line 626, its own expect() helper at line 40 incrementing g_failures, and return g_failures == 0 ? 0 : 1 as the only thing that decides the exit code. qExec/QTEST_MAIN is never called, so no QTestLog logger is ever installed. Two consequences, both read off the headers rather than inferred:
QTest::qVerify's failure path routes toQTestResult::addFailure→QTestLog::addFail, which iterates an empty logger list. Nothing prints, andg_failuresis untouched.QVERIFYexpands toif (!QTest::qVerify(...)) QTEST_FAIL_ACTION;, andQTEST_FAIL_ACTIONisdo { QTest::Internal::maybeThrowOnFail(); return; } while (false)(qtestcase.h:39; thethrowOnFailvariant needsQTEST_THROW_ON_FAIL, which is not defined, and the runtime flag it checks is set only byqExec). So it is a plainreturnfrom the void test function.
Combined: if qWaitForWindowExposed ever returns false, testDoubleClickOpensOneViewer() returns immediately, every expect() below it is skipped (248, 253, 258, 271, …), g_failures stays 0, and the process prints PASS and exits 0. That is strictly weaker than the pre-existing ignored return value, which at least let the case continue into its real assertions and trip g_failures there. For a PR filed under Principle VIII, these three sites make the failure quieter, not louder — and the three cases affected are the RFC #4603 guardrails (masked rows read-only, corrupt document not editable, array document treated as corrupt).
This also makes one sentence of the PR body false: "Each now asserts it through the test's own harness (QVERIFY, check or report)." QVERIFY is not this file's harness — expect() is, and this is the only file in tests/ that uses QVERIFY without qExec (the other 23 all go through QTEST_MAIN/qExec). The #6367 triage comment prescribed exactly that: "All three are hand-rolled int main() harnesses, so the fix follows each file's own convention rather than a Qt macro … the other two increment a g_failures counter." That triage landed ~9 minutes after this PR opened, so it could not have been known at authoring time — but it is the right call and matches what the file does.
Three-line drop-in, at 245, 290 and 355 respectively:
expect(QTest::qWaitForWindowExposed(&dlg), "the settings browser window is exposed");expect() is in the same anonymous namespace and in scope at all three sites, it records the failure and lets the case continue, so it preserves today's control flow exactly while actually satisfying the issue.
Nits
tests/qrz_callsign_test.cpp:189—check(f.open(), …)records the failure but does not return, sof.write(...)on line 190 then runs against an unopenedQTemporaryFile. Harmless (the write returns -1 and the parse assertions fail loudly afterwards) and strictly better than the status quo, so not worth a round trip — but aif (failures) return 1;style bail would make the first failure the only one reported.- The PR title says "GCC 14 deprecation" warnings.
QApplication::setActiveWindowis deprecated as of Qt 6.5 and warns on clang too — #6359's body independently reports it from a macOS build. The title slightly understates the reach; the body is accurate.
What I tried to break
- Sibling
[=]lambdas left behind.src/gui/RadioSetupDialog.cpphas two more at 7338 (refreshBitDetail) and 7508 (showCableProps). Neither referencesm_*orthis->, andprotoToSourceis a local lambda at 6876, not a member — so neither implicitly capturesthisand neither warns. Correctly left alone; the file is complete after this change. No other[=]exists in it. - The
QDragEnterEvent→QPointFclaim. My local Qt 6.11.2 header has onlyQDragEnterEvent(const QPoint &pos, …)with no QPointF overload, which would have made both hunks non-compiling churn. It isn't:CMakeLists.txt:353isfind_package(Qt6 6.12 REQUIRED)with an explicitVERSION_LESS 6.12fatal error at 342, so 6.12 is a hard floor, and the overload plus theQT_DEPRECATED_VERSION_X_6_16marker exist there. Qt bakesQT_WARN_DEPRECATED_UP_TOto0x070000unconditionally, so a 6.16 marker is live at a 6.12 build — the deprecation is real, not a 6.16 time bomb. (I had this backwards myself on #6367 and the thread already carries the correction.)stage_tab_bar_drag_test.cpp:74already passedQPointF(pos)to theQDropEventbeneath, so the pair now agrees. activateWindow()being a no-op under offscreen. This was my main worry:setActiveWindowbypassed the platform and set the active window synchronously, whileactivateWindow()asks the window system — and the two checks that follow depend on it (the Ctrl+Shift+F9QShortcutat ~1003 and the Alt+F mnemonic at ~1009 both needQApplication::activeWindow()forQShortcutMap::correctWidgetContext). In-tree precedent settles it:tests/window_shortcut_test.cpp:40useswindow.activateWindow()for exactly this shortcut-delivery purpose under offscreen, andtests/canon_window_test.cppuses it five times (119, 134, 148, 195, 222). No finding.- Argument order and counter wiring on the
check/reportswaps.check(bool, const char*)atqrz_callsign_test.cpp:29andreport(const char*, bool)atsystem_info_dialog_test.cpp:48— both call sites match their signature, both helpers increment a counter thatmainreturns. 3 of the 6 nodiscard sites are correctly wired; the 3 in Blocker 1 are not. - Security audit (CodeGuard) — ran as required over all eight flagged files: no findings. All 187 triage hits are false positives on lines this PR does not touch:
CS-VLN-CMDI-AMP/-BACKTICKare C++&&, Qt accelerator escapes ("Support && Diagnostics..."), and Markdown-quoted Flex verbs inside//comments;CS-VLN-SSRF-*andCS-PII-IPare screenshot alt text indocs/user/screens.jsonand a display fixture atunified_title_bar_test.cpp:436;CS-VLN-SSRF-HEXis the0x03…ullrequest-ID base;CS-SEC-AWS-SECRETis"sk-live-PROBE"atsettings_browser_dialog_test.cpp:188, a deliberate placeholder in the test that asserts[REDACTED]masking;CS-VLN-PATH-PROCis the literal/proc/self/status. The one production hunk has no security delta —fillis invoked only from theextensionResulthandler bound toguard = new QObject(this)(4336), so it cannot outlive the dialog either before or after. - Distrusting green. All five checks pass on
957abbc, but none of the six changed tests is named anywhere inci.yml— the onlyctest -Rsteps there cover the digital-voice set,cross_needle_meter_test,mac_nr_filter_testandasr_gpu_probe_test. So green proves the diff compiles at Qt 6.12 on Linux, macOS and Windows (which is itself the evidence that theQPointFoverload exists), and proves nothing about the runtime behaviour of any test change here. The lane that does run them isfull-suite.yml, which is not a PR gate. The body's "the test passes under the offscreen platform with it" rests on one local Linux run.
Everything above is reasoned from the diff, the head checkout at 957abbc, the installed Qt 6.11.2 headers and the #6367 thread. I have no build and no test run, so nothing here was reproduced at runtime; where that matters I have said which claim is unverified and what would settle it.
Recommendation
Request changes — one blocker, three lines, mechanical. The six other edits are correct, minimal, and exactly what #6367 asks for; the [=, this] and activateWindow() changes in particular I tried hard to break and could not. The settings_browser_dialog_test.cpp hunk is the one that needs another pass: as written it neither asserts the precondition nor fails the test, and it silently skips three guardrail cases on the failure path. Swap the three QVERIFY(...) for expect(..., "…") and this is ready. Worth confirming settings_browser_dialog and unified_title_bar_test green via full-suite before merge, since PR CI never executes either. Thanks for the careful per-line issue — the line numbers and warning attributions all held up against the tree.
🤖 aethersdr-agent · cost: $14.9989 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (192 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $15.1687 · model: claude-opus-5 |
…ct, not QVERIFY. Principle VIII. This file is a hand-rolled harness (expect + g_failures, int main), so QVERIFY cannot fail the run and only returns from the case. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks for the review. Pushed fba5ecf. Taken
Not taken
🤖 Generated with Claude Code |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Issue fit
Yes. #6367 enumerates the implicit-this capture, ignored file-open/window-exposure results, deprecated drag-enter constructors, and direct setActiveWindow call. Each maps to the corresponding changed hunk; the excluded atomic-pointer and Icom warnings stay outside this PR. The latest commit fixes the previous review's settings-harness blocker: all three exposure results now feed expect() and its process-exit failure counter.
Scope
| File/group | Change | Claimed? | Verdict |
|---|---|---|---|
src/gui/RadioSetupDialog.cpp:4337 |
Explicit this capture |
Yes | In scope; same captured pointer |
tests/qrz_callsign_test.cpp:189 |
Count temporary-file open failure | Yes | In scope |
tests/settings_browser_dialog_test.cpp:245,290,355 |
Count window-exposure failures in the local harness | Yes | In scope; previous blocker addressed |
tests/system_info_dialog_test.cpp:431,466 |
Count seed/rotated log open failures | Yes | In scope |
tests/stage_tab_bar_drag_test.cpp:70, tests/workspace_controller_test.cpp:93 |
Use available QPointF drag-enter constructor | Yes | In scope |
tests/unified_title_bar_test.cpp:982 |
Request activation with QWidget API | Yes | In scope; offscreen delivery tested |
Everything in the diff is explained by the issue. The three authored commits follow the warning cleanup and review correction; no unrelated commit, production default, UX preference, protocol surface, settings key or CHANGELOG entry. The checklist holds. No socket ownership or fake peer is added to the six tests; their Qt Network linkage alone does not constitute a socket test.
Blockers
None on current head fba5ecf8a. The prior request-changes review describes QVERIFY code that is no longer present.
Nits (non-blocking)
None requiring a code change. Failed open checks continue into the subsequent fixture operations and can produce secondary failures; that behavior is already disclosed and does not hide the primary failure.
What I tried to break
- Configured a scratch review build with pinned Qt 6.12.0, GCC 16.2.1, shared core and
-O2 -g1 -DNDEBUG. Built only the six selected test targets; ran the exact registered namesqrz_callsign_test,settings_browser_dialog,stage_tab_bar_drag_test,system_info_dialog_test,unified_title_bar_test, andworkspace_controller_test: 6/6 passed. The offscreen title-bar shortcut checks surviveactivateWindow(), and the real drag/drop tests survive the QPointF change. - Forced each newly tested precondition to return false in scratch source (three exposure results, one temporary-file open and two log opens). All three executables returned failure, with
[FAIL] the settings browser window is exposedthree times,[FAIL] temporary cty.dat file opens,[FAIL] the seed log file opens, and[FAIL] the rotated log file opens. This directly settles the corrected hand-rolled harness claim. Restored the exact head source, rebuilt, and confirmed 6/6 pass again. - Compiled the changed production
RadioSetupDialog.cppobject. Restoring the base capture spelling reproducesRadioSetupDialog.cpp:4337:21: warning: implicit capture of 'this' via '[=]' is deprecated in C++20; restoring the explicit capture removes that warning. Inspected the reply context parented to the dialog, request-ID filtering and QSignalBlockers: lifecycle and status reconciliation are unchanged. The other[=]lambdas do not capture members and correctly remain unchanged. - Read installed Qt 6.12 drag-event declarations, all three failure-counter helpers and final exit expressions, and the QPointF drop-event siblings. Checked base/head engine-boundary, capability, command-plane and touchpoint-manifest gates: all pass with byte-identical per-file outputs; no enforcement baseline or manifest grows.
git merge-treeagainst current main is clean.
The build uses shared core for the settings-dialog target, while its GUI sources compile directly into the executable. This does not claim a complete application link, a GCC 14 historical warning count, Windows/macOS runtime test execution or a hardware-settings reply round trip. No bridge app was launched: production changes only the spelling of an already captured pointer, and the changed widget/test behavior was driven by the actual socket-free tests. No operator instance or radio was touched. No separate automated review skill was available; the equivalent manual audit verified existing bot leads against the latest head. The initial configure encountered temporary-file quota; using a workspace TMPDIR resolved it, rather than attributing the environment failure to this PR.
All five reported PR checks succeeded, but none of these six tests is part of the filtered pre-merge runtime gate. Earliest head run creation is 2026-10-11T15:05:59Z, older than current main 0625d24, committed 2026-10-11T15:12:10Z. With non-strict protection, that green does not establish the current-main merge; local tests above are independent evidence.
Recommendation
Approve. The modified tests pass, deliberate precondition failures reach nonzero process exits, and the changed production translation unit compiles. Obtain CODEOWNERS review and refreshed integration checks; the old settings-harness finding is resolved by the current source.
There was a problem hiding this comment.
Landing approval at verified head fba5ecf.
fba5ecf: tests/settings_browser_dialog_test.cpp:245,290,355 uses expect(), fixing the old QVERIFY finding. All six changed tests pass; forcing all new preconditions false makes all three affected executables fail. A failed open is already a process failure, as the mutation demonstrates; subsequent fixture failures cannot turn it into a pass. The GCC14 title identifies the reported toolchain and makes no exclusivity claim; the body discloses Qt APIs and wider compiler effects.
The complete scope, governance, empirical evidence and unverified boundaries are recorded in my preceding review. I re-inventoried reviews, comments, every thread, current signatures and required checks: no unresolved thread, active CHANGES_REQUESTED review, or remaining actionable finding. No additional code push was needed. The manual adversarial pass substitutes for the unavailable automated skill; no operator app or radio was touched.
All four required checks pass on this head and it merges cleanly. Main at arming is 46d0f0e, newer than the PR run creation; strict:false deliberately permits this and the post-merge CI is the independent verification. No update solely for CI freshness was made.
Independent evidence: six registered tests pass after mutation restoration; the actual dialog object reproduces the implicit-this warning with base spelling and removes it with head spelling. Four boundary gates have identical base/head outputs. Full application linking and native Windows/macOS runtime were not repeated.
…s and silences their C++20 deprecation. Principle VIII. (#6369) ## Summary Fixes #6368. `TxCoordinator` publishes each operation's holder list with `std::atomic_store_explicit` and reads it with `std::atomic_load_explicit` on a `shared_ptr`. Those free functions are deprecated in C++20, so GCC 14 reports 34 `-Wdeprecated-declarations` (two sites, once per target that compiles the file). The calls stay, wrapped in `QT_WARNING_PUSH` / `QT_WARNING_DISABLE_DEPRECATED` / `QT_WARNING_POP` with a one-line comment. The replacement, `std::atomic<std::shared_ptr<T>>` (P0718R2), is not implemented by libc++ (its C++20 status page lists the row with no status and no release version), and the macOS jobs build with Apple clang on libc++, so migrating the field would break them. The pragma can be removed once libc++ ships it. **This is not the first time the project wraps a deprecated call this way.** `tests/TestWindowActivation.h` already does exactly this around `QApplication::setActiveWindow`, for the same reason: the supported replacement does not do the job on every platform we build for, so the deprecated call is kept and the warning is silenced where it is used. This PR follows that precedent. It is the first use of the wrapping in `src/`, which is why the comment at each site says why the free functions stay. No behaviour change: the calls, their memory orders and the field type are untouched. Not in this PR: `IcomModels.cpp`'s `-Wmissing-field-initializers`, and the dialog and test warnings in #6366. ## Constitution principle honored Principle VIII — Evidence Over Assertion: the premise (libc++ has no `std::atomic<std::shared_ptr>`) was checked against the libc++ status page before choosing suppression over migration. ## Test plan - [x] Local build passes (`cmake --build build`) - [ ] Behavior verified on a real radio if applicable - [ ] Existing tests pass (CI) - [ ] Reproduction steps documented if user-reported bug Local full build on Linux (GCC 14.2, Qt 6.12.0): no `TxCoordinator.cpp` warnings, and `tx_coordinator_test` passes. No radio was involved: no behaviour changes. CI has not run yet. ## Checklist - [x] Commits are signed (`docs/COMMIT-SIGNING.md`) - [x] No new flat-key `AppSettings` calls — use nested-JSON-under-one-key (Principle V) - [x] Code is clean-room — not decompiled, disassembled, or reverse-engineered from a proprietary binary (Principle IV) - [ ] All meter UI uses `MeterSmoother` (AGENTS.md convention) - [ ] Documentation updated if user-visible behavior changed — `docs/` and the affected READMEs. **Not `CHANGELOG.md`**, which is a release-prep file a PR must not add to (AGENTS.md); describe it in the Summary above instead - [ ] Security-sensitive changes reference a GHSA if applicable 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
2 participants
Summary
Fixes #6367. A clean GCC 14 build reported four project-owned warning groups that have a direct fix; this clears them with no production behaviour change.
RadioSetupDialog's hardware-settings reply lambda used[=], which implicitly capturesthis(deprecated in C++20). It is now[=, this], the same capture.[[nodiscard]]result (QFile::open,QTemporaryFile::open,QTest::qWaitForWindowExposed), so a failed open or a window that never exposed went unnoticed. Each now asserts it through that test's own hand-rolled harness (expect,checkorreport, each of which counts a failure toward the exit code;QVERIFYwould not, as these files have no QtTest main).QDragEnterEventfrom aQPoint, which Qt deprecates in favour of theQPointFconstructor.unified_title_bar_testcalledQApplication::setActiveWindowdirectly; it now callsQWidget::activateWindow(), Qt's own replacement (the test passes under the offscreen platform with it).Not in this PR:
TxCoordinator's deprecated atomicshared_ptrcalls (#6368) andIcomModels.cpp's missing-initializer warnings.Constitution principle honored
Principle VIII — Evidence Over Assertion: tests that could not fail on a failed precondition now assert it.
Test plan
cmake --build build)Local build on Linux (GCC 14.2, Qt 6.12.0): the app and the six touched test targets build with none of these warnings, and
qrz_callsign_test,settings_browser_dialog,stage_tab_bar_drag_test,system_info_dialog_test,unified_title_bar_testandworkspace_controller_testpass underQT_QPA_PLATFORM=offscreen. No radio was involved: no production behaviour changes. CI has not run yet.Checklist
docs/COMMIT-SIGNING.md)AppSettingscalls — use nested-JSON-under-one-key(Principle V)
reverse-engineered from a proprietary binary (Principle IV)
MeterSmoother(AGENTS.md convention)docs/and theaffected READMEs. Not
CHANGELOG.md, which is a release-prep file aPR must not add to (AGENTS.md); describe it in the Summary above instead
🤖 Generated with Claude Code