Support custom fixed point data type - #60
Conversation
📝 WalkthroughWalkthroughAdds fixed-point arithmetic and Q-format aliases, fixed-point Butterworth and Elliptic filter wrappers, conversion from floating-point filters, documentation, GoogleTest coverage, and GCC UBSan build support. ChangesFixed-point arithmetic and filters
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant FixedButterworth
participant Butterworth
participant FixedFilter
Caller->>FixedButterworth: provide design parameters
FixedButterworth->>Butterworth: compute double coefficients
FixedButterworth->>FixedFilter: convert coefficients to fixed-point
FixedFilter-->>Caller: return fixed-point filter
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/constfilt/fixed_point.hpp`:
- Around line 149-154: Document the non-zero divisor precondition for
FixedPoint::operator/ and the corresponding /= path, since rhs._raw being zero
causes runtime divide-by-zero undefined behavior. Update the operator/
implementation and any related in-place division logic or comments to clearly
state that the divisor must be non-zero, using the FixedPoint, operator/, and
operator/= symbols so the requirement is easy to find.
In `@tests/fixed_filter.test.cpp`:
- Around line 73-75: Fix the Q-format labels in the N=2 Butterworth comment so
they match the actual fixed-point types used in the tests. In the comment near
the N=2 section, replace the incorrect Q1.15 reference with Q0.15 for the [-1,1)
range, and replace Q1.31 with Q1.30 to match the Q1_30 alias; keep the rest of
the explanatory text consistent with the fixed-point range being described.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56664914-17bf-4d5c-bb2f-f893a65b750d
📒 Files selected for processing (7)
include/constfilt/constfilt.hppinclude/constfilt/filter.hppinclude/constfilt/fixed_filter.hppinclude/constfilt/fixed_point.hpptests/CMakeLists.txttests/fixed_filter.test.cpptests/fixed_point.test.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: cross.arm-clang
- GitHub Check: clang
- GitHub Check: gcc
- GitHub Check: cross.arm-gcc
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{hpp,h,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{hpp,h,cpp}: Use C++17 only; do not use C++20 features (no std::is_constant_evaluated, no concepts)
Enforce clang-format code formatting via CI; run make container.make.format before committing
Files:
include/constfilt/constfilt.hppinclude/constfilt/fixed_filter.hpptests/fixed_filter.test.cpptests/fixed_point.test.cppinclude/constfilt/filter.hppinclude/constfilt/fixed_point.hpp
include/constfilt/**
📄 CodeRabbit inference engine (AGENTS.md)
Maintain header-only library structure; do not add .cpp translation units to the library itself
Files:
include/constfilt/constfilt.hppinclude/constfilt/fixed_filter.hppinclude/constfilt/filter.hppinclude/constfilt/fixed_point.hpp
**/*.{hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
Implement constexpr everywhere possible; coefficient computation must work at compile time; Filter::operator() batch path must remain constexpr
Files:
include/constfilt/constfilt.hppinclude/constfilt/fixed_filter.hppinclude/constfilt/filter.hppinclude/constfilt/fixed_point.hpp
include/constfilt/**/*.{hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
include/constfilt/**/*.{hpp,h}: Do not introduce hard-coded filter-order ceiling; Filter classes should only use static_assert(N >= 1)
Use unsigned int for template parameter order N; implicit conversion with consteig::Size (size_t) is acceptable; do not add casts unless compiler error requires it
Follow a[0] = 1 convention for discrete transfer functions (monic-denominator); Filter stores _a with a[0] included for uniformity but DF2T equations assume a[0] = 1
Implement stability check via CheckStab template parameter (default true) to reject Stability::Unstable systems while accepting Stable and MarginallyStable; check not yet implemented (issue#14)
Use DirectForm II Transposed (DF2T) implementation for runtime filter with both sample-by-sample real-time path and constexpr-capable batch path
Support Butterworth filters with lowpass and highpass modes, arbitrary order (N >= 1), and ZOH or Matched-Z discretization options
Support Elliptic filters with lowpass and highpass modes, arbitrary order (N >= 1), and ZOH or Matched-Z discretization options; use Nome-series Cauer design (see docs/elliptic.md)
Files:
include/constfilt/constfilt.hppinclude/constfilt/fixed_filter.hppinclude/constfilt/filter.hppinclude/constfilt/fixed_point.hpp
**/*.hpp
⚙️ CodeRabbit configuration file
**/*.hpp: This is a constexpr C++17 header-only filter design library. Focus on:
- Correctness of constexpr computations and static_assert usage
- Type consistency (float vs double tolerances must match the filter's numeric type)
- No standard library dependencies in core library code (freestanding constraint)
- Correct use of vendored consteig (matrix ops) and gcem (math functions) via the wrapper
- Headers named *_reference.hpp contain precomputed test data — do not suggest algorithmic changes to their values
Files:
include/constfilt/constfilt.hppinclude/constfilt/fixed_filter.hppinclude/constfilt/filter.hppinclude/constfilt/fixed_point.hpp
**/tests/**
⚙️ CodeRabbit configuration file
**/tests/**: These are compile-time and runtime tests. Check:
- static_assert failures = build failures = test failures (treat equally)
- Tests are split into individual .cpp files intentionally to avoid constexpr budget exhaustion
- Do not suggest merging test files
- Reference values live in *_reference.hpp headers — do not suggest hardcoding tolerances inline
Files:
tests/fixed_filter.test.cpptests/CMakeLists.txttests/fixed_point.test.cpp
🪛 Cppcheck (2.21.0)
tests/fixed_filter.test.cpp
[error] 37-37: syntax error
(syntaxError)
tests/fixed_point.test.cpp
[error] 32-32: syntax error
(syntaxError)
🔇 Additional comments (11)
include/constfilt/filter.hpp (1)
43-54: LGTM!Also applies to: 57-63, 123-123
include/constfilt/fixed_filter.hpp (1)
1-30: LGTM!include/constfilt/constfilt.hpp (1)
9-10: LGTM!tests/CMakeLists.txt (1)
32-33: LGTM!tests/fixed_filter.test.cpp (1)
37-58: LGTM!Also applies to: 60-71, 77-98, 100-111, 115-129
tests/fixed_point.test.cpp (2)
32-32: The Cppcheck "syntax error" at this line is a false positive; thestatic constexpr Q15 x(0.5);construct is valid and Cppcheck simply mis-parses it without full type context. No change needed.
1-332: LGTM!include/constfilt/fixed_point.hpp (4)
10-34: LGTM!
55-127: LGTM!
156-201: LGTM!
206-231: LGTM!
MitchellThompkins
changed the title
Route +, -, unary -, +=, -= through an unsigned intermediate so Q0_31 and Q1_30 (TInt = int) wrap on overflow instead of hitting signed-integer-overflow UB. Adds Q31 wrap tests for add, sub, and unary minus.
operator* renormalized by flooring the arithmetic right shift, giving every product a small systematic negative bias that accumulates as DC offset in an IIR feedback loop. Add a half-LSB bias before the shift for round to nearest.
fixed_point.hpp and the test file header labeled the zero-integer-bit tier Q1.7/Q1.15/Q1.31, while fixed_point_aliases.hpp and the docs call it Q0_7/Q0_15/Q0_31 and use Q1_x for the widened tier. Update the comments to match the convention the aliases established.
In Q0 formats a[0]=1.0 is not representable and saturates to just under the format maximum; harmless since DF2T never reads a[0], but coeffs_a()[0] will not read back exactly 1.0. Also note that coeffs_b() should be range checked alongside coeffs_a(), since b can exceed [-1, 1) for highpass or elliptic-with-gain designs.
Q0_15{1} fails to compile: an int literal converts equally well to TInt and to double, so the call is ambiguous. Document the required explicit cast.
Six single-statement for loops were missing braces; add them for consistency across the new fixed-point files.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/fixed_filter.test.cpp`:
- Around line 36-47: The batch test array initialization loops in
tests/fixed_filter.test.cpp at lines 36-47 and 82-93 should use range-based for
loops (or std::fill) instead of manual indexing; update both q_in and d_in
initialization blocks, using the appropriate Q0_15 and Q1_30 value types while
preserving their existing values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 783f6933-3e0a-475e-ad80-a41507257bf3
📒 Files selected for processing (11)
README.mddocs/fixed-point.mddocs/getting-started.mddocs/mkdocs.ymlinclude/constfilt/constfilt.hppinclude/constfilt/fixed_butterworth.hppinclude/constfilt/fixed_elliptic.hppinclude/constfilt/fixed_point.hppinclude/constfilt/fixed_point_aliases.hpptests/fixed_filter.test.cpptests/fixed_point.test.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{hpp,h,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{hpp,h,cpp}: Use C++17 only; do not use C++20 features (no std::is_constant_evaluated, no concepts)
Enforce clang-format code formatting via CI; run make container.make.format before committing
Files:
include/constfilt/fixed_point_aliases.hppinclude/constfilt/constfilt.hppinclude/constfilt/fixed_butterworth.hppinclude/constfilt/fixed_elliptic.hpptests/fixed_filter.test.cppinclude/constfilt/fixed_point.hpptests/fixed_point.test.cpp
include/constfilt/**
📄 CodeRabbit inference engine (AGENTS.md)
Maintain header-only library structure; do not add .cpp translation units to the library itself
Files:
include/constfilt/fixed_point_aliases.hppinclude/constfilt/constfilt.hppinclude/constfilt/fixed_butterworth.hppinclude/constfilt/fixed_elliptic.hppinclude/constfilt/fixed_point.hpp
**/*.{hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
Implement constexpr everywhere possible; coefficient computation must work at compile time; Filter::operator() batch path must remain constexpr
Files:
include/constfilt/fixed_point_aliases.hppinclude/constfilt/constfilt.hppinclude/constfilt/fixed_butterworth.hppinclude/constfilt/fixed_elliptic.hppinclude/constfilt/fixed_point.hpp
include/constfilt/**/*.{hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
include/constfilt/**/*.{hpp,h}: Do not introduce hard-coded filter-order ceiling; Filter classes should only use static_assert(N >= 1)
Use unsigned int for template parameter order N; implicit conversion with consteig::Size (size_t) is acceptable; do not add casts unless compiler error requires it
Follow a[0] = 1 convention for discrete transfer functions (monic-denominator); Filter stores _a with a[0] included for uniformity but DF2T equations assume a[0] = 1
Implement stability check via CheckStab template parameter (default true) to reject Stability::Unstable systems while accepting Stable and MarginallyStable; check not yet implemented (issue#14)
Use DirectForm II Transposed (DF2T) implementation for runtime filter with both sample-by-sample real-time path and constexpr-capable batch path
Support Butterworth filters with lowpass and highpass modes, arbitrary order (N >= 1), and ZOH or Matched-Z discretization options
Support Elliptic filters with lowpass and highpass modes, arbitrary order (N >= 1), and ZOH or Matched-Z discretization options; use Nome-series Cauer design (see docs/elliptic.md)
Files:
include/constfilt/fixed_point_aliases.hppinclude/constfilt/constfilt.hppinclude/constfilt/fixed_butterworth.hppinclude/constfilt/fixed_elliptic.hppinclude/constfilt/fixed_point.hpp
**/*.hpp
⚙️ CodeRabbit configuration file
**/*.hpp: This is a constexpr C++17 header-only filter design library. Focus on:
- Correctness of constexpr computations and static_assert usage
- Type consistency (float vs double tolerances must match the filter's numeric type)
- No standard library dependencies in core library code (freestanding constraint)
- Correct use of vendored consteig (matrix ops) and gcem (math functions) via the wrapper
- Headers named *_reference.hpp contain precomputed test data — do not suggest algorithmic changes to their values
Files:
include/constfilt/fixed_point_aliases.hppinclude/constfilt/constfilt.hppinclude/constfilt/fixed_butterworth.hppinclude/constfilt/fixed_elliptic.hppinclude/constfilt/fixed_point.hpp
**/tests/**
⚙️ CodeRabbit configuration file
**/tests/**: These are compile-time and runtime tests. Check:
- static_assert failures = build failures = test failures (treat equally)
- Tests are split into individual .cpp files intentionally to avoid constexpr budget exhaustion
- Do not suggest merging test files
- Reference values live in *_reference.hpp headers — do not suggest hardcoding tolerances inline
Files:
tests/fixed_filter.test.cpptests/fixed_point.test.cpp
🪛 Cppcheck (2.21.0)
tests/fixed_filter.test.cpp
[error] 28-28: syntax error
(syntaxError)
🪛 markdownlint-cli2 (0.23.0)
docs/fixed-point.md
[warning] 114-114: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (10)
include/constfilt/fixed_butterworth.hpp (1)
1-36: LGTM!include/constfilt/fixed_elliptic.hpp (1)
1-30: LGTM!include/constfilt/constfilt.hpp (1)
9-10: LGTM!Also applies to: 13-13
README.md (1)
47-50: LGTM!docs/fixed-point.md (1)
114-114: 📐 Maintainability & Code Quality | 💤 Low valueSpecify a language for the fenced code block.
As highlighted by static analysis, this fenced code block is missing a language specification. Adding
cppwill improve syntax highlighting and resolve the lint warning.📝 Proposed fix
-``` +```cppSource: Linters/SAST tools
docs/getting-started.md (1)
119-140: LGTM!docs/mkdocs.yml (1)
48-48: LGTM!include/constfilt/fixed_point.hpp (1)
47-54: LGTM!Also applies to: 135-171, 182-197
include/constfilt/fixed_point_aliases.hpp (1)
1-30: LGTM!tests/fixed_point.test.cpp (1)
5-363: LGTM!
Adds a CONSTFILT_SANITIZE CMake option and a gcc-ubsan Makefile/CI target that builds and runs the test suite with -fsanitize=undefined. GCC-only: the dev container's Clang has no sanitizer runtimes for this musl target, and GCC's libasan is broken on musl in this image, so ASan and Clang sanitizer builds aren't available here. Verified the target catches a real signed-overflow regression and fails ctest.
Addresses CodeRabbit nitpick on PR #60: q_in/d_in are fixed-size C arrays, so range-based for works directly and is simpler than manual indexing since the loop body doesn't use the index.
Q0_15{static_cast<short>(1)} and Q0_15{1.0} produced different raw values (-32768 vs 32767) since 1 is not representable in Q0_15's [-1,1) range at all. Switch the primary example to Q1_14, where 1 is representable and both forms agree, and add a note on when they diverge. Verified all four raw values by compiling.
Filter never divides; the DF2T recurrence is multiply, add, and subtract only. Listing operator/ as required told custom scalar authors to implement an operator Filter never calls.
Adds fixed_point.test.cpp and fixed_filter.test.cpp to the layer table, explains why the fixed-point layer is checked against the double prototype instead of Octave directly, and adds tolerance rationale rows for Q0_15 and Q1_30.
|
Coefficients with an order of >3 can blow past the Q1 range (only 1 integer bit) and the double constructor just saturates them silently with no error nor warning. I confirmed this with Essentially if you choose a bad fixed point range and end up calculating a coefficient that can't fit into that range, and there's no way to know that today (other than the filter will mis-behave). Solutions for this fall under the same type of fix as #14 (which is to say I don't have a fix today) so instead I'm adding a comment over there and expanding the issue to include this check too. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
include/constfilt/fixed_point.hpp (1)
213-217: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSaturate the quotient before narrowing.
Line 215 can produce a value outside
TInt. ForFixedPoint<short, int, 15>, dividing raw-32768by0.5produces raw-65536. The conversion toshortis implementation-defined and can wrap instead of saturating.Clamp the widened quotient before constructing the result. Add boundary tests for both
/and/=.Proposed fix
constexpr FixedPoint operator/(FixedPoint rhs) const noexcept { - return {static_cast<TInt>((static_cast<TWider>(_raw) * SCALE) / - static_cast<TWider>(rhs._raw)), + const TWider quotient = + (static_cast<TWider>(_raw) * SCALE) / static_cast<TWider>(rhs._raw); + return {quotient >= static_cast<TWider>(INT_MAX_VAL) + ? INT_MAX_VAL + : (quotient <= static_cast<TWider>(INT_MIN_VAL) + ? INT_MIN_VAL + : static_cast<TInt>(quotient)), raw_tag{}}; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/constfilt/fixed_point.hpp` around lines 213 - 217, Update FixedPoint::operator/ to clamp the widened quotient to the TInt representable range before converting it to TInt and constructing the result, preserving normal quotient behavior within range. Apply the same saturation logic to operator/=, and add boundary tests covering overflow in both division operators.docs/fixed-point.md (2)
72-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify which type is replaced.
FixedButterworthandFixedEllipticuse the fixed-point type for stored and runtime coefficients, but their design arguments remain floating-point values, as shown by the examples. Replace “floating-point scalar type” with “coefficient storage type” to avoid suggesting that cutoff, ripple, or attenuation arguments should useTFixed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/fixed-point.md` around lines 72 - 76, Update the “FixedButterworth and FixedElliptic” documentation to replace “floating-point scalar type” with “coefficient storage type,” making clear that fixed-point applies to stored and runtime coefficients while design arguments remain floating-point values.
26-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winQualify compile-time conversion for non-
constexprobjects.The
constexprconstructors permit constant evaluation but do not require it. State indocs/fixed-point.mdanddocs/index.mdthat coefficient conversion occurs at compile time forconstexprfilter objects; non-constexprobjects can perform conversion during initialization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/fixed-point.md` around lines 26 - 28, The documentation statements about coefficient conversion must distinguish constexpr and non-constexpr filter objects. In docs/fixed-point.md lines 26-28, update the compile-time claim to apply to constexpr objects and state that non-constexpr objects may convert during initialization; make the equivalent clarification in docs/index.md lines 55-57, preserving the existing runtime arithmetic description.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/fixed-point.md`:
- Around line 93-96: Update the FixedFilter documentation to describe its
constructor prototype as Filter<U, NB, NA>, or equivalently as a floating-point
Filter, instead of limiting it to Filter<double, NB, NA>. Preserve the guidance
about converting an AnalogFilter and explicitly managing the double prototype.
- Line 193: Update the prose around std::numeric_limits to hyphenate
“floating-point” when it is used as a compound adjective, changing “floating
point one” accordingly.
- Around line 151-165: Make the integer constructor’s out-of-range behavior
explicit and portable across supported C++17 targets, rather than relying on
implementation-defined conversion to TInt. Update the constructor implementation
and its documentation/tests so signed overflow wraps deterministically to the
documented negative raw value, or explicitly restrict and qualify the behavior
for two’s-complement targets.
In `@include/constfilt/fixed_point.hpp`:
- Around line 199-203: Update the fixed-point multiplication implementation
around ROUND_BIAS to apply symmetric ties-away-from-zero rounding: add the bias
for nonnegative products and subtract it for negative products before shifting.
Match the rounding behavior used by to_raw(), and add constexpr coverage for
both positive and negative half-LSB multiplication cases.
---
Outside diff comments:
In `@docs/fixed-point.md`:
- Around line 72-76: Update the “FixedButterworth and FixedElliptic”
documentation to replace “floating-point scalar type” with “coefficient storage
type,” making clear that fixed-point applies to stored and runtime coefficients
while design arguments remain floating-point values.
- Around line 26-28: The documentation statements about coefficient conversion
must distinguish constexpr and non-constexpr filter objects. In
docs/fixed-point.md lines 26-28, update the compile-time claim to apply to
constexpr objects and state that non-constexpr objects may convert during
initialization; make the equivalent clarification in docs/index.md lines 55-57,
preserving the existing runtime arithmetic description.
In `@include/constfilt/fixed_point.hpp`:
- Around line 213-217: Update FixedPoint::operator/ to clamp the widened
quotient to the TInt representable range before converting it to TInt and
constructing the result, preserving normal quotient behavior within range. Apply
the same saturation logic to operator/=, and add boundary tests covering
overflow in both division operators.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b12fa70d-ebbf-4b4c-b1f4-83d460357d87
📒 Files selected for processing (5)
docs/fixed-point.mddocs/index.mddocs/verification.mdinclude/constfilt/fixed_point.hpptests/fixed_point.test.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: cross.arm-clang
- GitHub Check: clang
- GitHub Check: gcc-ubsan
- GitHub Check: cross.arm-gcc
- GitHub Check: gcc
- GitHub Check: cross.arm-gcc
- GitHub Check: gcc-ubsan
- GitHub Check: cross.arm-clang
- GitHub Check: gcc
- GitHub Check: clang
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{hpp,h,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{hpp,h,cpp}: Use C++17 only; do not use C++20 features (no std::is_constant_evaluated, no concepts)
Enforce clang-format code formatting via CI; run make container.make.format before committing
Files:
tests/fixed_point.test.cppinclude/constfilt/fixed_point.hpp
**/tests/**
⚙️ CodeRabbit configuration file
**/tests/**: These are compile-time and runtime tests. Check:
- static_assert failures = build failures = test failures (treat equally)
- Tests are split into individual .cpp files intentionally to avoid constexpr budget exhaustion
- Do not suggest merging test files
- Reference values live in *_reference.hpp headers — do not suggest hardcoding tolerances inline
Files:
tests/fixed_point.test.cpp
include/constfilt/**
📄 CodeRabbit inference engine (AGENTS.md)
Maintain header-only library structure; do not add .cpp translation units to the library itself
Files:
include/constfilt/fixed_point.hpp
**/*.{hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
Implement constexpr everywhere possible; coefficient computation must work at compile time; Filter::operator() batch path must remain constexpr
Files:
include/constfilt/fixed_point.hpp
include/constfilt/**/*.{hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
include/constfilt/**/*.{hpp,h}: Do not introduce hard-coded filter-order ceiling; Filter classes should only use static_assert(N >= 1)
Use unsigned int for template parameter order N; implicit conversion with consteig::Size (size_t) is acceptable; do not add casts unless compiler error requires it
Follow a[0] = 1 convention for discrete transfer functions (monic-denominator); Filter stores _a with a[0] included for uniformity but DF2T equations assume a[0] = 1
Implement stability check via CheckStab template parameter (default true) to reject Stability::Unstable systems while accepting Stable and MarginallyStable; check not yet implemented (issue#14)
Use DirectForm II Transposed (DF2T) implementation for runtime filter with both sample-by-sample real-time path and constexpr-capable batch path
Support Butterworth filters with lowpass and highpass modes, arbitrary order (N >= 1), and ZOH or Matched-Z discretization options
Support Elliptic filters with lowpass and highpass modes, arbitrary order (N >= 1), and ZOH or Matched-Z discretization options; use Nome-series Cauer design (see docs/elliptic.md)
Files:
include/constfilt/fixed_point.hpp
**/*.hpp
⚙️ CodeRabbit configuration file
**/*.hpp: This is a constexpr C++17 header-only filter design library. Focus on:
- Correctness of constexpr computations and static_assert usage
- Type consistency (float vs double tolerances must match the filter's numeric type)
- No standard library dependencies in core library code (freestanding constraint)
- Correct use of vendored consteig (matrix ops) and gcem (math functions) via the wrapper
- Headers named *_reference.hpp contain precomputed test data — do not suggest algorithmic changes to their values
Files:
include/constfilt/fixed_point.hpp
🪛 LanguageTool
docs/fixed-point.md
[grammar] ~193-~193: Use a hyphen to join words.
Context: ...eger convention rather than the floating point one std::numeric_limits uses. `m...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (4)
docs/fixed-point.md (3)
36-40: 🗄️ Data Integrity & IntegrationDocument silent saturation for every coefficient.
The text only calls out saturation of
a[0].FixedPoint(double)also saturates any out-of-rangea[k]orb[k]. For example, a coefficient near2.4becomes2.0inQ1_30, which can change the filter response without a diagnostic. Add this limitation explicitly and direct users to validate both coefficient arrays before construction.
1-25: LGTM!Also applies to: 30-35, 42-71, 78-90, 98-150, 167-174, 181-186, 188-189, 191-192, 194-198
175-180: Test negative multiplication rounding on all CI compilers.Add a C++17 compile-time test for negative signed right shifts on GCC, Clang, ARM GCC, and ARM Clang. If any target differs, implement explicit sign-aware rounding.
docs/verification.md (1)
18-19: LGTM!Also applies to: 29-38, 56-57
| `FixedFilter` is the lower-level building block. It converts any | ||
| `Filter<double, NB, NA>` to a fixed-point filter by taking the double filter as | ||
| a constructor argument. Use it when you need to convert an `AnalogFilter`, or when you prefer to | ||
| manage the double prototype explicitly. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the actual prototype type accepted by FixedFilter.
The constructor accepts Filter<U, NB, NA> for a templated U; it is not limited to Filter<double, NB, NA>. Use Filter<U, NB, NA> or “a floating-point Filter” in this section.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/fixed-point.md` around lines 93 - 96, Update the FixedFilter
documentation to describe its constructor prototype as Filter<U, NB, NA>, or
equivalently as a floating-point Filter, instead of limiting it to
Filter<double, NB, NA>. Preserve the guidance about converting an AnalogFilter
and explicitly managing the double prototype.
| An integer literal such as `Q1_14{1}` fails to compile: it converts equally | ||
| well to `TInt` (the integer constructor) and to `double` (the double | ||
| constructor), so the call is ambiguous. Cast the literal to the storage type | ||
| or to `double` to disambiguate, for example `Q1_14{static_cast<short>(1)}` or | ||
| `Q1_14{1.0}` (both produce raw value `16384`, the representation of `1.0` in | ||
| `Q1_14`'s `[-2, 2)` range). | ||
|
|
||
| The two forms are not interchangeable in general, and picking the wrong one | ||
| matters most near a format's limits. The integer constructor scales its | ||
| argument by `2^FracBits` and wraps on overflow; the double constructor | ||
| rounds to the nearest representable value and saturates at the format's | ||
| limits. In a `Q0` format (range `[-1, 1)`) the value `1` is not representable | ||
| at all, so `Q0_15{static_cast<short>(1)}` wraps to a negative raw value while | ||
| `Q0_15{1.0}` saturates to just under the maximum. Pick the form that matches | ||
| what the value actually means. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'integer_val|static_cast<TInt>.*SCALE' include/constfilt/fixed_point.hppRepository: MitchellThompkins/constfilt
Length of output: 1487
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- fixed_point.hpp relevant implementation ---'
sed -n '80,145p' include/constfilt/fixed_point.hpp
printf '%s\n' '--- project standard and target assumptions ---'
rg -n -C 3 'CXX_STANDARD|cxx_std_17|std=c\+\+17|CMAKE_CXX|two.?complement|wrap|integer constructor|Wraps silently' \
CMakeLists.txt cmake include test tests docs 2>/dev/null || true
printf '%s\n' '--- uses and tests for the integer constructor ---'
rg -n -C 5 'FixedPoint<|Q0_15|Q1_14|raw_value\(\)|wrap|overflow' \
test tests include docs 2>/dev/null || trueRepository: MitchellThompkins/constfilt
Length of output: 50385
🌐 Web query:
C++17 [conv.integral] out-of-range conversion to signed integer implementation-defined signal
💡 Result:
Under the C++17 standard, specifically section [conv.integral], when an integer value is converted to a signed integer type and the value cannot be represented in the destination type, the resulting value is implementation-defined [1][2]. Unlike the C standard (specifically C11, section 6.3.1.3), which explicitly states that such a conversion results in either an implementation-defined value or an implementation-defined signal [3], the C++17 standard does not mention signals [1][2]. It simply specifies that the result is implementation-defined [1][2]. In practice, virtually all modern C++ implementations define this behavior as a silent wraparound (e.g., using two's complement representation) [4][5], though the standard requires that the specific behavior be documented by the implementation [6].
Citations:
- 1: https://standards.pydong.org/c++17/conv.integral
- 2: https://timsong-cpp.github.io/cppwp/std17/conv.integral
- 3: https://stackoverflow.com/questions/47767253/is-printfd-n-inta-undefined-behaviour-for-unsigned-int
- 4: https://stackoverflow.com/questions/60179125/assigning-an-out-of-range-integer-to-a-signed-char-type
- 5: https://stackoverflow.com/questions/71267644/conversion-to-signed-type-behavior-when-out-of-range
- 6: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1407r1.pdf
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- focused integer-construction tests ---'
sed -n '24,72p' tests/fixed_point.test.cpp
rg -n -C 4 'FromInteger|static_cast<short>|static_cast<signed char>|integer_val|Q0_15.*1|Q1_14.*1' \
tests/fixed_point.test.cpp docs/fixed-point.md include/constfilt/fixed_point.hpp
printf '%s\n' '--- supported target documentation ---'
rg -n -C 3 'ARM|target|platform|two.?complement|signed integer|supported' \
README.md docs CMakeLists.txt cmake/toolchains include/constfilt/fixed_point.hpp 2>/dev/null | head -n 180Repository: MitchellThompkins/constfilt
Length of output: 19046
Define signed wrapping for the integer constructor.
C++17 makes an out-of-range conversion to TInt implementation-defined. The documented negative raw value is not portable across the project’s C++17 targets. Implement explicit wrapping, or qualify the documentation and tests for supported two’s-complement targets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/fixed-point.md` around lines 151 - 165, Make the integer constructor’s
out-of-range behavior explicit and portable across supported C++17 targets,
rather than relying on implementation-defined conversion to TInt. Update the
constructor implementation and its documentation/tests so signed overflow wraps
deterministically to the documented negative raw value, or explicitly restrict
and qualify the behavior for two’s-complement targets.
Sources: Coding guidelines, Path instructions
|
|
||
| `fixed_limits<TInt, TWider, FracBits>` provides `min()`, `max()`, `lowest()`, | ||
| and `epsilon()` without requiring `<limits>`. This follows the integer | ||
| convention rather than the floating point one `std::numeric_limits` uses. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use floating-point as a compound adjective.
Change “floating point one” to “floating-point one.”
🧰 Tools
🪛 LanguageTool
[grammar] ~193-~193: Use a hyphen to join words.
Context: ...eger convention rather than the floating point one std::numeric_limits uses. `m...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/fixed-point.md` at line 193, Update the prose around std::numeric_limits
to hyphenate “floating-point” when it is used as a compound adjective, changing
“floating point one” accordingly.
Source: Linters/SAST tools
| constexpr TWider ROUND_BIAS = TWider(1) << (FracBits - 1); | ||
| const TWider wide = static_cast<TWider>( | ||
| (static_cast<TWider>(_raw) * static_cast<TWider>(rhs._raw) + | ||
| ROUND_BIAS) >> | ||
| FracBits); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use symmetric rounding for negative products.
Line 201 adds ROUND_BIAS for both signs. A negative half-LSB product then rounds toward zero. For example, from_raw(1) * from_raw(-16384) in Q0.15 produces raw 0, while the positive equivalent produces raw 1.
Use the same ties-away-from-zero rule as to_raw(). Add a constexpr test for positive and negative half-LSB products.
Proposed fix
-const TWider wide = static_cast<TWider>(
- (static_cast<TWider>(_raw) * static_cast<TWider>(rhs._raw) +
- ROUND_BIAS) >>
- FracBits);
+const TWider product =
+ static_cast<TWider>(_raw) * static_cast<TWider>(rhs._raw);
+const TWider quotient = product / SCALE;
+const TWider remainder = product % SCALE;
+const TWider wide = remainder >= ROUND_BIAS ? quotient + 1
+ : remainder <= -ROUND_BIAS ? quotient - 1
+ : quotient;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constexpr TWider ROUND_BIAS = TWider(1) << (FracBits - 1); | |
| const TWider wide = static_cast<TWider>( | |
| (static_cast<TWider>(_raw) * static_cast<TWider>(rhs._raw) + | |
| ROUND_BIAS) >> | |
| FracBits); | |
| constexpr TWider ROUND_BIAS = TWider(1) << (FracBits - 1); | |
| const TWider product = | |
| static_cast<TWider>(_raw) * static_cast<TWider>(rhs._raw); | |
| const TWider quotient = product / SCALE; | |
| const TWider remainder = product % SCALE; | |
| const TWider wide = remainder >= ROUND_BIAS ? quotient + 1 | |
| : remainder <= -ROUND_BIAS ? quotient - 1 | |
| : quotient; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@include/constfilt/fixed_point.hpp` around lines 199 - 203, Update the
fixed-point multiplication implementation around ROUND_BIAS to apply symmetric
ties-away-from-zero rounding: add the bias for nonnegative products and subtract
it for negative products before shifting. Match the rounding behavior used by
to_raw(), and add constexpr coverage for both positive and negative half-LSB
multiplication cases.
1 participant
Summary by CodeRabbit