Hold TapTools' own code to the family's warning standard; export tap::tools - #26
Merged
Merged
Conversation
…:tools TapTools was the one library in the family with no warnings interface target and no WERROR option -- every sibling carries the pair (AMBITAP_WERROR / TAP_DSP_WERROR / MUTAP_WERROR / TAP_RATIO_WERROR / SRT_WERROR) -- so its kernel compiled without -Wall -Wextra -Wpedantic -Wconversion -Wshadow while everything around it did not. Adds `taptools_warnings` and `TAPTOOLS_WERROR` with exactly the flag set the siblings use, and links the test target against it. Turning them on surfaced 42 warnings. Two were in a shipping header and are fixed here: vco.h's `tri_tick` took a `dt` parameter it never used and that both call sites already filled with `adt`, and `waveform_out_peek` took an `adt` it has no use for -- peek reads the triangle integrator rather than ticking it, so it adds no BLEP correction, which is now stated where the parameter used to be. Both are private helpers of a nested class with no callers outside this header, so removing the vestigial parameters is a no-op for behavior: the suite still passes 1637526 assertions across 156 test cases. The remaining 34 are -Wconversion in test files. WERROR is therefore left OFF and not yet enabled in CI -- the same staged approach the siblings took with MSVC /W4, where the flag waits until the output has been triaged. Also exports `tap::tools`, the alias taphouse's namespace convention documented but this repo never created (`TapTools::taptools` stays, so consumers are unaffected); fixes the book's site-url, which pointed at /TapTools/book/ while docs.yml serves the book from /TapTools/, giving mdBook's generated 404.html a broken home link; adds book/book/ to .gitignore, which AmbiTap and MuTap already ignore and which a local mdbook build otherwise leaves untracked; and corrects a header comment that still named the pre-split `taptools` namespace. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JhhQ93r2E1QTnCx46YfX8j
The style job's clang-format gate failed at include/taptools/vco.h:470. Removing waveform_out_peek's unused `adt` parameter shortened both calls, so the manual continuation-line alignment on that statement no longer matched what clang-format produces; it now fits differently and the formatter wraps it after the `=`. My miss: I ran clang-format over the .cpp files I touched in TapTools-Max but not over this repo's include/*.h, which its style job also globs. Now verified by reproducing the exact CI invocation -- `clang-format --dry-run --Werror` over `git ls-files 'include/*.h' 'tests/*.cpp' 'tools/*.cpp' 'tools/*.h' 'bench/*.cpp'` with clang-format 18.1.3, the pinned version -- clean across the whole set. Formatting only: rebuilt and re-ran the suite, still 1637526 assertions across 156 test cases, and include/taptools/ still compiles warning-free under the new taptools_warnings flags. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JhhQ93r2E1QTnCx46YfX8j
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Adds a
taptools_warningsinterface target and aTAPTOOLS_WERRORoption, links the test target against it, and fixes the two warnings it surfaced in a shipping header. Also exports thetap::toolsCMake alias, fixes the book'ssite-url, addsbook/book/to.gitignore, and corrects a stale namespace comment.Why
TapTools was the one library in the family with no warnings target and no WERROR option. Every sibling carries the pair —
AMBITAP_WERROR,TAP_DSP_WERROR,MUTAP_WERROR,TAP_RATIO_WERROR,SRT_WERROR— so the kernel that every TapTools-Max external compiles against was building without-Wall -Wextra -Wpedantic -Wconversion -Wshadowwhile everything around it was not. The flag set here is deliberately identical to the siblings' rather than a fresh choice.The
tap::toolsalias is the one taphouse's namespace convention has documented all along but this repo never created (see tap/TapHouse#6).Verification
vco.h) and are fixed here; the remaining 34 are-Wconversionin test files.include/taptools/warning-free: header warnings 42 → 0, test-file warnings 34.ctest1/1.docs.yml), and the generated404.htmlnow links/TapTools/instead of the non-existent/TapTools/book/..gitignorerule was verified by creating a file insidebook/book/and confirminggit check-ignore -vmatches it.TAPTOOLS_WERRORis left OFF and is not enabled in CI. 34 test-file warnings remain, so switching it on would break the build. This is the same staged approach the siblings took with MSVC/W4— the flag exists and the warnings are now visible; enabling it waits until the output is triaged.Notes for the reviewer
The two
vco.hfixes are vestigial-parameter removals, and both are worth a glance to confirm you agree they are dead rather than intended:tri_tick(double p, double dt, double adt, double tri_pw)never useddt, and both call sites already passedadtfor it (tri_tick(p, adt, adt, tri_pw)).waveform_out_peek(double p, double adt, …)never usedadt— correctly, since peek reads the triangle integrator rather than ticking it, so it adds no BLEP correction and has no use for the window width. That reasoning is now a comment where the parameter used to be.Both are private helpers of a nested class with no callers outside this header, so removing the parameters cannot affect any consumer.
CMakeLists.txtstill named the pre-splittaptoolsnamespace; the actual namespace has beentap::toolssince the repo split.TapTools::taptoolsstays alongside the newtap::tools, so no consumer changes. Note the alias is a build-tree target: the installed config package still exports underTapTools::, sofind_packageconsumers see the old spelling until that is migrated too.Generated by Claude Code