Repository navigation
chore(build): TxCoordinator keeps the shared_ptr atomic free functions and silences their C++20 deprecation. Principle VIII. - #6369
Conversation
…s and silences their C++20 deprecation. Principle VIII. OperationState::holds is published with std::atomic_store_explicit and read with std::atomic_load_explicit on a shared_ptr. Those free functions are deprecated in C++20, so GCC 14 reported 34 -Wdeprecated-declarations (two sites, once per target that compiles TxCoordinator.cpp). The replacement, std::atomic<std::shared_ptr<T>> (P0718R2), is not implemented by libc++, and the macOS jobs build with Apple clang on libc++, so migrating the field would break them. The two calls stay and are wrapped in QT_WARNING_DISABLE_DEPRECATED, the same wrapping tests/TestWindowActivation.h already uses around QApplication::setActiveWindow. This is the first use of it in src/. No behaviour change: the calls, their memory orders and the field type are untouched. Verified on Linux (GCC 14.2, Qt 6.12.0): a full build reports no TxCoordinator.cpp warnings and tx_coordinator_test passes. Fixes aethersdr#6368 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
Yes. #6368 asks for exactly this: keep the two std::atomic_*_explicit calls on OperationState::holds, silence -Wdeprecated-declarations locally, and leave a comment saying why so the suppression can be removed when libc++ ships std::atomic<std::shared_ptr>. Both sites named in the issue (:122 read, :821 write — now :126 and :829) are wrapped, and the diff does nothing else. The issue's non-goal ("the code is correct, do not migrate the field") is honoured: no - lines, no type change, memory orders untouched.
I checked the body's central factual claim rather than taking it: src/core/TxCoordinator.h:380 declares holds as a plain std::shared_ptr<const std::vector<std::weak_ptr<IntentState>>>, and in the libc++ headers I can read here (CommandLineTools MacOSX.sdk __memory/shared_ptr.h:1505-1516) there is only the free-function family and no atomic<shared_ptr> specialization anywhere in the tree — so "migrating the field would break the macOS build" holds.
The body's precedent claim also holds, which is worth recording because it is load-bearing for the style: tests/TestWindowActivation.h:33-36 already wraps QApplication::setActiveWindow in the same QT_WARNING_PUSH / QT_WARNING_DISABLE_DEPRECATED / QT_WARNING_POP triple, column-0 pragmas and all, for the same reason. This is the first use in src/, not in the project.
Scope
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/TxCoordinator.cpp:4 |
#include <QtGlobal> |
yes | In scope — reaches the macros: QtGlobal → qglobal.h:31 → qcompilerdetection.h |
src/core/TxCoordinator.cpp:123-127 |
comment + push/disable/pop around the atomic_load_explicit read |
yes (#6368 site 1) | In scope |
src/core/TxCoordinator.cpp:826-832 |
comment + push/disable/pop around the atomic_store_explicit write |
yes (#6368 site 2) | In scope |
Everything in the diff is explained by #6368. One file, three pragma lines, two comment lines, one include; no new public surface, no settings key, no capability field, no CHANGELOG.md entry, nothing deleted.
Two context notes for you, neither a finding:
- The triage comment on #6368 declined it as "CI/CD workflows, build infrastructure, or release packaging … outside what I can help with, restricted to
src/anddocs/". The fix landed entirely insrc/core/, so there is no actual out-of-scope ruling standing against this PR — the bot classified the symptom (build warnings) rather than the change. - security-audit (CodeGuard): no finding on any line this PR changes. All 58 flagged findings sit in
docs/user/screens.jsonandsrc/gui/MainWindow*.cpp, none of which this PR touches, and the audit refuted each category in place (&&inMainWindow_Menus.cpp:385is a Qt mnemonic escape; theMainWindow.cppbackticks are Markdown-quoted Flex verbs inside//comments; thescreens.jsonaddresses are RFC 5737 TEST-NET-1 andyou@example.com). Nothing to carry forward.
Blockers
None.
Nits (non-blocking)
-
src/core/TxCoordinator.cpp:123and:826— the comment says why the call stays but not when the suppression goes away, and does not cite the issue:// libc++ has no std::atomic<std::shared_ptr>, so the free functions stay.#6368's stated acceptance is "so it can be removed once libc++ ships
std::atomic<std::shared_ptr>", and the precedent attests/TestWindowActivation.h:9-27spends twenty lines on the measured reason and the condition for removal. A grep foratomic<std::shared_ptr>will find these two lines, but nobody will find the reasoning or the issue. One extra clause at each site:// #6368: libc++ has no std::atomic<std::shared_ptr> (P0718R2), so the free // functions stay. Drop the push/pop once libc++ implements it.
-
Same two sites — a precision point about what the suppression actually does. In the libc++ on this machine the free functions carry no deprecation attribute at all (
__memory/shared_ptr.h:1505,:1514:_LIBCPP_HIDE_FROM_ABIand nothing else), so today the pragma is inert on the macOS/clang arm and the 34 warnings it clears are GCC/libstdc++ only. That is fine — libstdc++ both deprecates the free functions and implements the replacement, which is exactly why the warning is Linux-only and the migration is still blocked — and the push/pop is already in place if a newer libc++ adds the marker. Worth a word in the comment only if you take the nit above.
What I tried to break
- "These are the only two sites." Grepped the whole head checkout for
atomic_(load|store|exchange|compare_exchange): exactlyTxCoordinator.cpp:126and:829, plustests/thumbdv_queue_test.c:62,123, which are C11atomic_loadon anintflag and carry no C++20 deprecation. No sibling site left unwrapped, and no non-atomic read or write ofholdsanywhere — a plainoperation->holdsaccess alongside the atomic ones would have been a real data race, and there is none. - Over-broad suppression. Both regions are
PUSH/DISABLE/ one statement /POP, balanced, and wrap a single expression each — the second covers the full three-lineatomic_store_explicitcall and stops. Nothing else in these functions is masked, and no deprecation introduced later in the file would be swallowed. - Wrong flag on some compiler.
QT_WARNING_DISABLE_DEPRECATEDresolves toQT_WARNING_DISABLE_MSVC(4996),-Wdeprecated-declarationsfor clang (plus-Wdeprecated-pragma), and-Wdeprecated-declarationsfor GCC (qcompilerdetection.h:1199,:1215,:1226), and to nothing on other compilers. Correct on all three CI lanes, and the include reaches them on all three. - Behaviour change hiding in the include.
<QtGlobal>is macro- and declaration-only andqglobal.hwas already reachable through<QScopeGuard>; the only semantic content in the diff is three preprocessor lines. The claim "the calls, their memory orders and the field type are untouched" is verifiable from the diff itself —acquireon the read,releaseon the write, both unchanged. - The precedent claim. The body leans on
tests/TestWindowActivation.hexisting and doing "exactly this". It does, on this branch, unmodified by this PR — so the column-0 pragma placement matches house precedent and is not something to ask changed. (The rawQApplication::setActiveWindowstill attests/unified_title_bar_test.cpp:982is #6366's business, not this PR's.) - What I could not check. I cannot build or run anything, so the "34 → 0 warnings on GCC 14.2" measurement is the author's local claim; I confirmed the mechanism, not the count. All five checks on
d88d0b3are green, but there is no-WerrorinCMakeLists.txtorci.yml, so green only ever proved it compiles — it never proved the warning was present and it does not prove it is gone. No test is expected or possible here (a compiler-warning count is not a CTest subject), and no socket test is added or removed by this PR. Every statement above is reasoned from the code in the head checkout, not reproduced at runtime.
Recommendation
Approve with nits. This is the narrowest change that satisfies #6368: the two sites the issue names, wrapped tightly, with the migration correctly rejected for a reason I was able to verify in the libc++ headers rather than take on faith. The one thing genuinely for you rather than the author is the precedent — this is the first warning suppression in src/, and the existing one in tests/ is a test fixture, so accepting it here sets the pattern for the next deprecated-but-unreplaceable call. If you would rather src/ stayed suppression-free, the alternative is a __cpp_lib_atomic_shared_ptr conditional on the member type, which forks the field's type by platform and is a markedly worse trade for two call sites. Otherwise: take the comment nit so the pragma has a removal trigger attached, and merge. Thanks for splitting this out of #6366 and for citing the libc++ status explicitly instead of asserting it.
🤖 aethersdr-agent · cost: $10.9280 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (63 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: $11.2141 · model: claude-opus-5 |
…ordinator atomic suppression. Principle VIII. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks for the review. Pushed 5551d22, comment-only. Taken
The warning itself is not re-measured here (no GCC on this machine); CI's Ubuntu lane covers it. 🤖 Generated with Claude Code |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Issue fit
Yes. #6368 asks to keep the shared_ptr atomic free functions, suppress their deprecations locally, and document the libc++ constraint/removal trigger. Both read and publish sites receive balanced push/disable/pop regions; the calls, field type, acquire/release orders, and TX lifecycle code remain unchanged.
Scope
| File/group | Change | Claimed? | Verdict |
|---|---|---|---|
src/core/TxCoordinator.cpp:4 |
Explicit Qt macro include | Yes | In scope |
src/core/TxCoordinator.cpp:123-129 |
Scoped deprecation suppression on atomic read | Yes | In scope |
src/core/TxCoordinator.cpp:828-836 |
Scoped deprecation suppression on atomic publication | Yes | In scope |
Everything in the diff is explained by #6368. Both commits' author dates fit this change. No guard, call, public API, setting, default, user documentation obligation, or CHANGELOG entry changes; the scope checklist holds. GOVERNANCE's build-fix exception applies; no new architecture or UX decision is needed.
Blockers
None.
Nits (non-blocking)
None.
What I tried to break
- Compiled the actual coordinator and unchanged
tx_coordinator_test.cppon merge base and head with GCC 16.2.1, Qt Core, C++20,-fPIC -O0 -Wall -Wextra -Wpedantic. Base emits the two shared_ptr atomic deprecations; head emits zero. This independently measures the central warning claim, though not the historical 34-instance GCC 14 count. - Ran both resulting socket-free test executables: 441 PASS, zero FAIL on each. These exercise ownership/recovery, invalid/stale authority, callback reentrancy, handoff, intent holds, and request capacity; no radio or synthetic peer is involved. No test source was added or changed, so no new regression-test mutation claim is made.
- Checked for sibling atomic sites and plain accesses to
OperationState::holds: only these two atomic accesses remain. Both suppressions surround one expression and restore diagnostics immediately. No executable statement or memory order is changed. - Verified the portability premise against the official libc++ C++20 status table: P0718R2 still has no completion status or released version. Checked
tests/TestWindowActivation.hfor the stated Qt suppression precedent. CodeGuard's findings target unrelated files; none identifies an introduced sink or changed statement here. An invocable automated code-review skill was unavailable; this was a manual audit. - Did not drive the app: the pragma/include change has no runtime UI path, and coordinator behavior was exercised directly. Named CMake build initially failed on environment restrictions (ccache read-only cache, then temporary-file quota); compiling the same test sources directly with Qt-required PIC succeeded. This is a standalone test compile, not a completed application build. No operator app instance or radio was touched.
CI qualification: head runs were created 2026-10-11T15:05:59Z with pull_request; current main was committed 2026-10-11T15:12:10Z. The checks describe the earlier merge snapshot. Non-strict branch updating does not force a rerun.
Recommendation
Approve. The warning suppression and unchanged socket-free TX policy behavior are independently verified, and no blocker emerged. Required checks/CODEOWNERS still govern landing; validate the combination with current main rather than treating stale green as proof of that combination.
There was a problem hiding this comment.
Landing approval at verified head 5551d22.
5551d22: both TxCoordinator.cpp suppression comments name #6368, the libstdc++ warning and the libc++ removal trigger. Actual coordinator base/head builds measured two GCC deprecations versus zero, with 441 PASS and zero FAIL on each.
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 de05122, 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.
The direct socket-free test compile exercises the real coordinator; this is not a full application build. Atomic calls, pointer type, acquire/release orders and executable statements are unchanged.
Summary
Fixes #6368.
TxCoordinatorpublishes each operation's holder list withstd::atomic_store_explicitand reads it withstd::atomic_load_expliciton ashared_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_POPwith 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.halready does exactly this aroundQApplication::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 insrc/, 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
cmake --build build)Local full build on Linux (GCC 14.2, Qt 6.12.0): no
TxCoordinator.cppwarnings, andtx_coordinator_testpasses. No radio was involved: no 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