fix(audio): replace unreachable fallback in primaryRateOrder with Q_UNREACHABLE (#3432)#3433
Conversation
…NREACHABLE (aethersdr#3432) The trailing `return {internalRate}` after both direction/OS switch statements was dead code — every TargetOs value is covered in both the Output and Input branches. It silently masked future omissions: adding a new TargetOs enum value without updating a switch would compile cleanly and return the wrong rate list. Replace with Q_UNREACHABLE() so the compiler/runtime enforces the invariant instead. All 25 audio_format_negotiation_test checks pass.
There was a problem hiding this comment.
Thanks for the cleanup, @M7HNF-Ian — the rationale is exactly right. The trailing return {internalRate} was a silent landmine: adding a fourth TargetOs value would compile cleanly and quietly return only the internal rate, breaking the per-OS ladder in a way that no test or warning would catch. Turning it into a hard invariant via Q_UNREACHABLE() is the right move.
A few notes for awareness (none blocking):
-
First
Q_UNREACHABLE()in this codebase. I grepped and there are no existing uses. The neighbourtoString(TargetOs)(AudioFormatNegotiator.cpp:228) andtoString(ResamplerKind)(line 218) still use silentreturn "?"defaults. That's fine — the divergence is justified becauseprimaryRateOrder()feeds a real negotiation path while thetoStringdefaults are debug-only — but worth flagging in case the project later wants to standardise on one pattern. -
Q_UNREACHABLE()vsQ_UNREACHABLE_RETURN(...): Qt 6 ships both. PlainQ_UNREACHABLE()should be sufficient with Clang/GCC/MSVC since it expands to__builtin_unreachable()/__assume(0), but if any CI compiler is older or treats the macro defensively, you may see a "control reaches end of non-void function" warning. If that surfaces, swap toQ_UNREACHABLE_RETURN({})to suppress it without weakening the invariant. Not needed pre-emptively. -
Test coverage: confirming locally that all 25
audio_format_negotiation_testchecks pass is exactly the right validation — the function is pure-policy by design (per the file header comment), so unit tests are the appropriate gate.
LGTM as a defensive correctness improvement. Nice catch.
🤖 aethersdr-agent · cost: $6.9176 · model: claude-opus-4-7
The trailing
return {internalRate}after both direction/OS switch statementsin
primaryRateOrder()is dead code — everyTargetOsvalue is covered inboth the Output and Input branches. It silently masks future omissions: adding
a new
TargetOsenum value without updating a switch would compile cleanlyand return the wrong rate list with no diagnostic.
Replace with
Q_UNREACHABLE()so the invariant ("everyTargetOsvalue ishandled in both directions") is enforced by the compiler/runtime rather than
papered over with a silent default.
Testing
All 25
audio_format_negotiation_testchecks pass (built and run locally onmacOS against the modified
AudioFormatNegotiator.cpp).Closes #3432