chore: update KissFFT paths #151
Conversation
|
Can you please also update the internal copy of kissfft using this command? https://github.com/acoustid/chromaprint/blob/master/src/3rdparty/update_kissfft.sh |
febd4ca Merge pull request #99 from lanurmi/newline 26992ad Add newline to end of kiss_fft_log.h 00cc50e Merge pull request #98 from felixjogris/bsdmake 1c3c9a5 call $(MAKE) instead of make f5f2a3b Merge pull request #70 from razor7788/work 59c8cdf fixed test/Makefile line 137 missing ')' 8f47a67 removed kfc.h and kissfft_i32.hh from the install list since they are not as generally useful as the others 0a43940 no benefit to having both a README and README.md -- D.R.Y. 478c35e Update documentation 7c9a558 testkiss.py: Do not depend on tools/fft_* 7eeda21 Add testsuite cccf41a Overhaul CMake makefiles 2e2747c Overhaul Make makefiles 7811f7d fixed switch-case fall-thru e321f7a dropped -Waggregate-return from warning flags as it created spurious warnings for recent c++ compilation (found through cmake build on CentOS 8) d11f1a8 Merge pull request #57 from basilgello/debian-fixes f88d5af Make kfc,fftnd,fftndr,fftr part of public API c2e82a3 Build and install kissfft-tools 7651bda Increase CTest interval cc862cb Merge pull request #53 from madebr/makefile_AR_CC_variables 15debe8 Merge pull request #52 from madebr/cmake_install 1487038 Use find_package COMPONENTS to select shared/static + datatype 2d5de83 Print datatype while configuring cmake a63dfc7 Build tools + tests with cmake + add VERSION + parallel install 609845b cmake more work fcf9fa9 Use $(CC) and $(AR) in Makefile 5c708d8 Add cmake install + MSVC dll support 0a70dfd Merge pull request #50 from seeker-Liu/master 4e4a738 Fix typo in document b2e0e60 Calling exit from library code is icky. Closes issue #49 c524b0b Merge pull request #47 from JulienMaille/patch-1 efff86a Fix conversion from 'double' to 'float' warning closes 159874b Merge pull request #46 from ademuri/master 6fcc85d Add support for CMake f0a47de Merge pull request #45 from kpawlak/task-logging-and-check 5fb101e Testting buffers for NULL befor usage 7c47cc9 Add common logging mechanism, use NDEBUG to turn off logging cf5813a added test for simd crash to "make testall" and thus travis CI e53d6d4 changed tabs to spaces for consistency and fix compiler warnings. Removed assert call (would require assert.h inclusion somewhere). 07ccd81 Merge remote-tracking branch 'xplane/master' into simd_misaligned b30b93c A test case for issue #40. Segfaults with: make -C test test_simd && ./test/test_simd 58cf0c0 alignment fixing for USE_SIMD - all memory allocations are rounded up to 16-byte boundaries. This fixes crashes where the sub-buffers for complex FFTs end up unaligned violating compiler expectations 33d9ad3 include guard for kiss guts - fixes compile on unity build systems c2c0c0b Merge pull request #38 from jtojnar/packagability b39de32 build: Make datatype configurable 1d7e76e build: Use install command for installing files fffc26e build: Fix make install on Darwin 4193457 build: Make installation prefix configurable 3f3fc6a upgrade to newer dist 592bdf7 want travis to run with python 2.7(for now) and 3.7 b420613 I had to fix some python3 incompatibilities and realized how embarrassing the code was. I refactored to make it look a little more like it was written by someone who knows Python. 1efe720 above commit was cherry-picked from MoonchildProductions fork of kissfft (closes issue #26) f4496dc Prevent stack corruption with parallelized use. 3050076 Merge pull request #28 from bmcdonnell-ionx/user-override-malloc d06e7a7 Allow user override of KISS_FFT_MALLOC and/or KISS_FFT_FREE on SIMD platforms 033f759 Allow user override of KISS_FFT_MALLOC and/or KISS_FFT_FREE on non-SIMD platforms 24d23ff whitespace b24d807 removed unused and rotted code (closes #25) 36dbc05 Merge pull request #22 from detly/master d1f9113 Include stdint.h only where fixed point types are used. Use min/max values from stdint.h. a75f084 Merge pull request #21 from jmcph4/fix20 bd55024 Include `stdint.h` in `_kiss_fft_guts.h` git-subtree-dir: src/3rdparty/kissfft git-subtree-split: febd4caeed32e33ad8b2e0bb5ea77542c40f18ec
WalkthroughThis update overhauls the kissfft third-party library integration. It introduces semantic versioning, new CMake and Makefile build systems with datatype and OpenMP configurability, and robust install/test targets. Symbol visibility is now controlled with Changes
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Done, note that I changed it from tag v131 to the latest commit SHA since it doesnt seem they set any tags past v131. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
🔭 Outside diff range comments (2)
cmake/modules/FindKissFFT.cmake (1)
63-66: 🧹 Nitpick (assertive)
tools/still added toKISSFFT_INCLUDE_DIRS– probably harmless but redundant.If upstream keeps
tools/only for helper binaries, adding it to public include dirs just bloats the search path.
You can safely trim it:- SET(KISSFFT_INCLUDE_DIRS - ${KISSFFT_SOURCE_DIR} - ${KISSFFT_SOURCE_DIR}/tools - ) + SET(KISSFFT_INCLUDE_DIRS + ${KISSFFT_SOURCE_DIR} + )src/3rdparty/kissfft/kissfft.pc.in (1)
1-11: 🧹 Nitpick (assertive)Pkg-config template: placeholder mismatch risk
@PKGCONFIG_KISSFFT_PKGINCLUDEDIR@is different fromincludedirand@PKGCONFIG_KISSFFT_INCLUDEDIR@. Double-check that the configure step sets both or change to one canonical placeholder to avoid an empty-Iflag.-Cflags: -I@PKGCONFIG_KISSFFT_PKGINCLUDEDIR@ @PKG_KISSFFT_DEFS@ +Cflags: -I${includedir} @PKG_KISSFFT_DEFS@
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (38)
cmake/modules/FindKissFFT.cmake(2 hunks)src/3rdparty/kissfft/.gitignore(2 hunks)src/3rdparty/kissfft/.hgignore(0 hunks)src/3rdparty/kissfft/.travis.yml(2 hunks)src/3rdparty/kissfft/CMakeLists.txt(1 hunks)src/3rdparty/kissfft/Makefile(1 hunks)src/3rdparty/kissfft/README(0 hunks)src/3rdparty/kissfft/README.md(1 hunks)src/3rdparty/kissfft/_kiss_fft_guts.h(5 hunks)src/3rdparty/kissfft/cmake/JoinPaths.cmake(1 hunks)src/3rdparty/kissfft/kfc.h(1 hunks)src/3rdparty/kissfft/kiss_fft.c(6 hunks)src/3rdparty/kissfft/kiss_fft.h(5 hunks)src/3rdparty/kissfft/kiss_fft_log.h(1 hunks)src/3rdparty/kissfft/kiss_fftnd.c(1 hunks)src/3rdparty/kissfft/kiss_fftnd.h(1 hunks)src/3rdparty/kissfft/kiss_fftndr.c(1 hunks)src/3rdparty/kissfft/kiss_fftndr.h(2 hunks)src/3rdparty/kissfft/kiss_fftr.c(5 hunks)src/3rdparty/kissfft/kiss_fftr.h(1 hunks)src/3rdparty/kissfft/kissfft-config.cmake.in(1 hunks)src/3rdparty/kissfft/kissfft.hh(1 hunks)src/3rdparty/kissfft/kissfft.pc.in(1 hunks)src/3rdparty/kissfft/test/CMakeLists.txt(1 hunks)src/3rdparty/kissfft/test/Makefile(1 hunks)src/3rdparty/kissfft/test/compfft.py(0 hunks)src/3rdparty/kissfft/test/fastfir.py(0 hunks)src/3rdparty/kissfft/test/fft.py(0 hunks)src/3rdparty/kissfft/test/kissfft-testsuite.sh(1 hunks)src/3rdparty/kissfft/test/tailscrap.m(0 hunks)src/3rdparty/kissfft/test/test_simd.c(1 hunks)src/3rdparty/kissfft/test/test_vs_dft.c(0 hunks)src/3rdparty/kissfft/test/testkiss.py(1 hunks)src/3rdparty/kissfft/tools/CMakeLists.txt(1 hunks)src/3rdparty/kissfft/tools/Makefile(1 hunks)src/3rdparty/kissfft/tools/psdpng.c(1 hunks)src/3rdparty/update_kissfft.sh(1 hunks)src/fft_lib_kissfft.h(1 hunks)
💤 Files with no reviewable changes (7)
- src/3rdparty/kissfft/.hgignore
- src/3rdparty/kissfft/test/fastfir.py
- src/3rdparty/kissfft/test/tailscrap.m
- src/3rdparty/kissfft/test/test_vs_dft.c
- src/3rdparty/kissfft/test/compfft.py
- src/3rdparty/kissfft/test/fft.py
- src/3rdparty/kissfft/README
🧰 Additional context used
🪛 LanguageTool
src/3rdparty/kissfft/README.md
[uncategorized] ~49-~49: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ... is float. ## BUILDING: There are two functionally-equivalent build systems supported by kissfft: -...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[misspelling] ~64-~64: This word is normally spelled as one.
Context: ...nMP supported by GCC, Clang or MSVC for multi-core FFT transformations Environments like ...
(EN_COMPOUNDS_MULTI_CORE)
[uncategorized] ~67-~67: “to the” seems less likely than “to”.
Context: ...s platform, but no tests were performed to the date. Both Make and CMake builds are e...
(AI_HYDRA_LEO_CP_TO_THE_TO)
[uncategorized] ~82-~82: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...te that a supported compiler is required and this option is turned off by default...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~86-~86: Possible missing article found.
Context: ...ake) instructs the builder to create static library ('.lib' for Windows / '.a' for ...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~95-~95: Did you mean: “By default,”?
Context: ...out command-line tools like 'fastconv'. By default the tools are built. - `KISSFFT_US...
(BY_DEFAULT_COMMA)
[typographical] ~159-~159: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...hat the extended testsuite takes around 20-40 minutes depending on device it runs on....
(HYPHEN_TO_EN)
[uncategorized] ~159-~159: Possible missing comma found.
Context: ...e extended testsuite takes around 20-40 minutes depending on device it runs on. This te...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~159-~159: Possible missing article found.
Context: ...takes around 20-40 minutes depending on device it runs on. This testsuite is useful fo...
(AI_HYDRA_LEO_MISSING_THE)
🪛 markdownlint-cli2 (0.17.2)
src/3rdparty/kissfft/README.md
47-47: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
51-51: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
61-61: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
62-62: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
63-63: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
64-64: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
71-71: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
75-75: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
76-76: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
77-77: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
78-78: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
79-79: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
81-81: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
85-85: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
90-90: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
94-94: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
97-97: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
100-100: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
107-107: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
113-113: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
121-121: Multiple consecutive blank lines
Expected: 1; Actual: 2
(MD012, no-multiple-blanks)
122-122: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
135-135: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
140-140: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
146-146: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
155-155: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 Shellcheck (0.10.0)
src/3rdparty/kissfft/test/kissfft-testsuite.sh
[info] 189-189: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 190-190: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 196-196: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 210-210: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 216-216: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 238-238: Double quote to prevent globbing and word splitting.
(SC2086)
🪛 YAMLlint (1.37.1)
src/3rdparty/kissfft/.travis.yml
[error] 21-21: wrong indentation: expected 8 but found 6
(indentation)
[error] 25-25: wrong indentation: expected 8 but found 6
(indentation)
🪛 Ruff (0.11.9)
src/3rdparty/kissfft/test/testkiss.py
7-7: Unnecessary __future__ imports absolute_import, division, print_function for target Python version
Remove unnecessary __future__ import
(UP010)
11-11: random imported but unused
Remove unused import: random
(F401)
12-12: struct imported but unused
Remove unused import: struct
(F401)
41-41: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
41-41: Use f-string instead of format call
Convert to f-string
(UP032)
44-44: Missing return type annotation for public function dopack
(ANN201)
44-44: Missing type annotation for function argument x
(ANN001)
51-51: Missing return type annotation for public function dounpack
(ANN201)
51-51: Missing type annotation for function argument x
(ANN001)
51-51: Missing type annotation for function argument cpx
(ANN001)
57-57: Missing return type annotation for public function make_random
(ANN201)
57-57: Missing type annotation for function argument shape
(ANN001)
61-61: Unnecessary else after return statement
Remove unnecessary else
(RET505)
64-64: Missing return type annotation for public function randmat
(ANN201)
64-64: Missing type annotation for function argument ndim
(ANN001)
71-71: Missing return type annotation for public function test_fft
Add return type annotation: None
(ANN201)
71-71: Missing type annotation for function argument ndim
(ANN001)
74-77: Use ternary operator xver = np.fft.rfftn(x) if do_real else np.fft.fftn(x) instead of if-else-block
Replace if-else-block with xver = np.fft.rfftn(x) if do_real else np.fft.fftn(x)
(SIM108)
86-86: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
86-86: Use f-string instead of format call
Convert to f-string
(UP032)
94-94: Missing return type annotation for public function dofft
(ANN201)
94-94: Missing type annotation for function argument x
(ANN001)
94-94: Missing type annotation for function argument isreal
(ANN001)
94-94: Unused function argument: isreal
(ARG001)
114-114: subprocess call with shell=True identified, security issue
(S602)
128-128: Missing return type annotation for public function main
Add return type annotation: None
(ANN201)
🪛 Pylint (3.3.7)
src/3rdparty/kissfft/test/testkiss.py
[error] 14-14: Unable to import 'numpy'
(E0401)
[convention] 16-16: Constant name "po" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 17-17: Constant name "e" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 18-18: Constant name "do_real" doesn't conform to UPPER_CASE naming style
(C0103)
[refactor] 24-24: Consider merging these comparisons with 'in' by using 'openmp in ('1', 'ON')'. Use a set instead if elements are hashable.
(R1714)
[convention] 27-27: Constant name "minsnr" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 34-34: Constant name "minsnr" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 41-41: Formatting a regular string which could be an f-string
(C0209)
[convention] 44-44: Missing function or method docstring
(C0116)
[convention] 51-51: Missing function or method docstring
(C0116)
[refactor] 59-62: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[convention] 71-71: Missing function or method docstring
(C0116)
[convention] 86-86: Formatting a regular string which could be an f-string
(C0209)
[convention] 94-94: Missing function or method docstring
(C0116)
[convention] 113-113: Import outside toplevel (subprocess.Popen, subprocess.PIPE)
(C0415)
[refactor] 114-114: Consider using 'with' for resource-allocating operations
(R1732)
[warning] 94-94: Unused argument 'isreal'
(W0613)
[convention] 128-128: Missing function or method docstring
(C0116)
[warning] 131-131: Using the global statement
(W0603)
[warning] 129-129: Unused variable 'args'
(W0612)
[warning] 11-11: Unused import random
(W0611)
[warning] 12-12: Unused import struct
(W0611)
🪛 checkmake (0.2.2)
src/3rdparty/kissfft/tools/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 53-53: Target "all" should be declared PHONY.
(phonydeclared)
src/3rdparty/kissfft/Makefile
[warning] 214-214: Target body for "install" exceeds allowed length of 5 (7).
(maxbodylength)
[warning] 269-269: Target body for "testall" exceeds allowed length of 5 (22).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 196-196: Target "all" should be declared PHONY.
(phonydeclared)
[warning] 315-315: Target "asm" should be declared PHONY.
(phonydeclared)
src/3rdparty/kissfft/test/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 111-111: Target "all" should be declared PHONY.
(phonydeclared)
[warning] 168-168: Target "test" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (23)
src/3rdparty/kissfft/tools/psdpng.c (1)
41-50: Good catch – fall-through fixedThe added
break;prevents the unintended drop into thedefaultbranch.
Behaviour now matches the usage intent.src/fft_lib_kissfft.h (1)
7-8: Verify include search paths after path changeSwapping
#include <tools/kiss_fftr.h>for#include <kiss_fftr.h>assumes the build system now addssrc/3rdparty/kissfftto-I.
Double-check CMake/Makefile targets (and downstream packagers) pick this up, otherwise include resolution will fail.src/3rdparty/update_kissfft.sh (1)
5-7: Script reproducibility looks fine – pin is a hard commitUsing the commit hash instead of a floating tag is safer.
Still, after the subtree pull you might want to add a conventional commit message (-m) for clarity.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
[info] 5-5: Double quote to prevent globbing and word splitting.
(SC2086)
src/3rdparty/kissfft/.gitignore (1)
4-5: Ignore patterns updated sensibly
*.so.*,test/test_simd, andbuild/are legitimate artefacts to drop from VCS. No issues here.cmake/modules/FindKissFFT.cmake (1)
48-50: Header/source lookup path update looks correct.
kiss_fftr.h / .creally live in the repo root after the upstream move, so dropping thetools/prefix is the right call.
No functional issues spotted here.src/3rdparty/kissfft/kissfft.hh (1)
128-131: Comment fix is spot-on.The doc string now matches the actual storage layout (
dst[0].real()/dst[0].imag()).
Nothing else to flag.src/3rdparty/kissfft/kfc.h (1)
42-48:KISS_FFT_APImacro looks fine hereThe header already includes
kiss_fft.h, which definesKISS_FFT_API, so the visibility decoration is safe. No further action required.src/3rdparty/kissfft/kiss_fftnd.h (1)
20-22: Consistent symbol‐visibility decoration—goodBoth declarations now carry
KISS_FFT_API, matching the rest of the public API headers. No issues spotted.src/3rdparty/kissfft/kiss_fft.c (2)
337-344: Alignment macros inserted correctly
KISS_FFT_ALIGN_CHECKandKISS_FFT_ALIGN_SIZE_UPwrap the allocation logic without altering semantics. Looks good.
205-210:❓ Verification inconclusive
Early return on OOM silently corrupts caller state
kf_bfly_genericlogs andreturns whenscratchisNULL, but all callers assume the function succeeded and will later read/writeFout. That leaves the transform half-baked with no error signal.- if (scratch == NULL){ - KISS_FFT_ERROR("Memory allocation failed."); - return; - } + if (!scratch) { + KISS_FFT_ERROR("Memory allocation failed."); + /* propagate failure */ + return; /* TODO: redesign API to bubble an error code upward */ + }At minimum document this undefined result; ideally change the API to return
intso the failure can be detected.
🏁 Script executed:
#!/bin/bash # Show definition of kf_bfly_generic with context rg -n -C5 "kf_bfly_generic" src/3rdparty/kissfft/kiss_fft.c # Show all references to kf_bfly_generic in the codebase rg -n "kf_bfly_generic"Length of output: 1880
🏁 Script executed:
#!/bin/bash # Locate KISS_FFT_TMP_ALLOC definition rg -n -C3 "KISS_FFT_TMP_ALLOC" src/3rdparty/kissfft # Locate KISS_FFT_ERROR definition rg -n -C3 "KISS_FFT_ERROR" src/3rdparty/kissfftLength of output: 5143
Review and handle potential OOM paths in kf_bfly_generic
By default
KISS_FFT_TMP_ALLOCmaps toalloca(), soscratch == NULLnever fires under normal builds—but if you disable alloca and fall back tomalloc, OOM will trigger that branch, log an error and return from avoidfunction, leavingFouthalf-updated with no error propagated.Action items:
- In
src/3rdparty/kissfft/kiss_fft.c(lines 205–210), document that on allocation failure the output buffer is left in an undefined state.- If you need true OOM safety, consider changing
static void kf_bfly_generic(...)to return anint(0 on success, –1 on failure) and update all callers (including inkissfft.hhandkissfft_i32.hh) to check it.- Alternatively, remove the unreachable NULL check when using
alloca()and assert instead to catch stack overflow early.src/3rdparty/kissfft/kiss_fftr.c (2)
23-24: Alignment macro added—good catch
KISS_FFT_ALIGN_CHECKbefore any pointer arithmetic prevents UB on mis-aligned custom buffers.
149-152: SIMD branch negates in place—OKNegating with
_mm_set1_ps(-1.0)matches the scalar path. No issues.src/3rdparty/kissfft/tools/CMakeLists.txt (1)
15-24:psdpngmakes the whole build hard-fail whenlibpngis missing
pkg_check_modules(libpng REQUIRED …)turns the optionalpsdpngtool into a mandatory dependency.
Iflibpngis absent the entire configure step bails out, even though every other target could build just fine.- include(FindPkgConfig) - pkg_check_modules(libpng REQUIRED IMPORTED_TARGET libpng) + include(FindPkgConfig) + pkg_check_modules(libpng QUIET IMPORTED_TARGET libpng) + if (libpng_FOUND) + set(PNG_TARGET PkgConfig::libpng) + else() + message(STATUS "libpng not found – skipping psdpng tool") + set(PNG_TARGET "") + endif()And wrap the
add_kissfft_executable(psdpng …)/install()block inif (PNG_TARGET).This keeps the build usable out-of-the-box on machines that don’t have
libpnglying around.
[ suggest_optional_refactor ]src/3rdparty/kissfft/README.md (2)
47-56: Docs: heading has a colon – markdown-lint complains
## BUILDING:ends with:which triggers MD026.
Drop the colon, the heading still reads fine:-## BUILDING: +## BUILDING(Same remark for
## TESTING:later on.)
[ suggest_nitpick ]🧰 Tools
🪛 LanguageTool
[uncategorized] ~49-~49: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ... is float. ## BUILDING: There are two functionally-equivalent build systems supported by kissfft: -...(HYPHENATED_LY_ADVERB_ADJECTIVE)
🪛 markdownlint-cli2 (0.17.2)
47-47: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
51-51: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
64-68: Wording nit – “to the date” → “to date”The current sentence:
“…but no tests were performed to the date.”
Correct idiom is “to date” (or “so far”).
[ suggest_nitpick ]🧰 Tools
🪛 LanguageTool
[misspelling] ~64-~64: This word is normally spelled as one.
Context: ...nMP supported by GCC, Clang or MSVC for multi-core FFT transformations Environments like ...(EN_COMPOUNDS_MULTI_CORE)
[uncategorized] ~67-~67: “to the” seems less likely than “to”.
Context: ...s platform, but no tests were performed to the date. Both Make and CMake builds are e...(AI_HYDRA_LEO_CP_TO_THE_TO)
🪛 markdownlint-cli2 (0.17.2)
64-64: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
src/3rdparty/kissfft/kiss_fftr.h (2)
9-11: Typo in include-guard macro – missing an ‘F’
#ifndef KISS_FTR_H→ should beKISS_FFTR_H, otherwise multiple inclusion protection breaks when another file defines the correct guard.-#ifndef KISS_FTR_H -#define KISS_FTR_H +#ifndef KISS_FFTR_H +#define KISS_FFTR_H[ raise_critical_issue ]
29-44:KISS_FFT_APIis in the wrong place – won’t compile on MSVC/clangVisibility attribute must precede the return type, e.g.
KISS_FFT_API kiss_fftr_cfg kiss_fftr_alloc(...);Today it’s
kiss_fftr_cfg KISS_FFT_API …, which MSVC treats as two unrelated tokens and rejects.Same for
kiss_fftr,kiss_fftri. Quick patch:-kiss_fftr_cfg KISS_FFT_API kiss_fftr_alloc( +KISS_FFT_API kiss_fftr_cfg kiss_fftr_alloc( … -void KISS_FFT_API kiss_fftr( +KISS_FFT_API void kiss_fftr( … -void KISS_FFT_API kiss_fftri( +KISS_FFT_API void kiss_fftri([ raise_critical_issue ]
src/3rdparty/kissfft/kiss_fftnd.c (2)
22-24: Alignment check occurs before memory is allocated
KISS_FFT_ALIGN_CHECK(mem)runs even whenmem == NULL.
On most builds the macro is astatic_assert/ifthat quietly passes, but on debug builds that log mis-alignment this prints a false warning.Move the check to the branch where
ptris finalised.[ suggest_essential_refactor ]
84-88: Size-t down-cast risks overflowif ( ptr - (char*)st != (int)memneeded )On 64-bit the calculated size can exceed
INT_MAX, silently wrapping and tripping the check. Compare assize_t:-if ( ptr - (char*)st != (int)memneeded ) { +if ((size_t)(ptr - (char*)st) != memneeded) {[ suggest_optional_refactor ]
src/3rdparty/kissfft/kiss_fftndr.h (1)
23-44: Visibility macro placement same issue askiss_fftr.hPut
KISS_FFT_APIbefore the return type for all three declarations:-kiss_fftndr_cfg KISS_FFT_API kiss_fftndr_alloc( +KISS_FFT_API kiss_fftndr_cfg kiss_fftndr_alloc( -void KISS_FFT_API kiss_fftndr( +KISS_FFT_API void kiss_fftndr( -void KISS_FFT_API kiss_fftndri( +KISS_FFT_API void kiss_fftndri([ raise_critical_issue ]
src/3rdparty/kissfft/kiss_fft.h (1)
117-130: Double-check attribute placement for MSVC.MSVC accepts
__declspecbefore the return type more reliably than after.
PlacingKISS_FFT_APIafter the return type works with GCC/Clang but has caused
warning C4190 on older MSVC releases.Consider:
-KISS_FFT_API kiss_fft_cfg kiss_fft_alloc(…); +KISS_FFT_API +kiss_fft_cfg kiss_fft_alloc(…);Same for the other prototypes. If your CI doesn’t build with MSVC you might miss this.
src/3rdparty/kissfft/test/Makefile (2)
129-130:❌ Incorrect review comment
Update source path for
fftutil.c
The filefftutil.cis no longer intools/. Change the dependency to point to the new location:-$(TESTFASTFILT): ../tools/fftutil.c +$(TESTFASTFILT): ../fftutil.cVerify this matches the tree after running
src/3rdparty/update_kissfft.sh.
Let’s verify where fftutil.c actually lives now:
🏁 Script executed:
#!/bin/bash # Locate fftutil.c in the repository fd fftutil.cLength of output: 51
fftutil.c path is correct in test Makefile
I ranfd fftutil.cand it’s still located atsrc/3rdparty/kissfft/tools/fftutil.c. The existing../tools/fftutil.cdependency insrc/3rdparty/kissfft/test/Makefileis accurate—no change required.Likely an incorrect or invalid review comment.
13-13:✅ Verification successful
Remove obsolete include path
-I../tools
KissFFT headers have been moved to the root; drop-I../toolsfrom your compile flags to avoid missing-header errors.
Diff:-CFLAGS = -O3 -I.. -I../tools $(WARNINGS) +CFLAGS = -O3 -I.. $(WARNINGS)Please confirm no other targets or tests still reference headers under
tools/.
Let’s verify there are no lingering references to the old tools folder:
🏁 Script executed:
#!/bin/bash echo "Checking for '-I../tools' usages:" rg -n "\-I\.\./tools" src/3rdparty/kissfft echo echo "Scanning for any 'tools/' paths in source:" rg -n "tools/" src/3rdparty/kissfft echo echo "Looking for include directives pulling from tools:" rg -n "#include .*tools" src/3rdparty/kissfftLength of output: 1486
Let’s inspect the test directory and its source to be sure no code still pulls in headers from
tools/:
🏁 Script executed:
#!/bin/bash echo "1) Listing test directory contents:" ls src/3rdparty/kissfft/test echo echo "2) Showing include directives in test .c files:" rg -n "#include" src/3rdparty/kissfft/test/*.c || true echo echo "3) Excerpt from test Makefile around CFLAGS:" sed -n '1,30p' src/3rdparty/kissfft/test/MakefileLength of output: 3751
Remove obsolete
-I../toolsfrom test MakefileAll test sources were scanned—none include headers from
tools/. You can safely drop the extra include path:--- a/src/3rdparty/kissfft/test/Makefile +++ b/src/3rdparty/kissfft/test/Makefile @@ -13,7 +13,6 @@ -CFLAGS = -O3 -I.. -I../tools $(WARNINGS) +CFLAGS = -O3 -I.. $(WARNINGS) CFLAGS += -ffast-math -fomit-frame-pointer
| #define ERROR 1 | ||
| #define WARNING 2 | ||
| #define INFO 3 | ||
| #define DEBUG 4 | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
ERROR / WARNING macro names collide with system headers
Windows headers, glib, and a handful of libraries already define ERROR, INFO, etc.
A single include can silently overwrite or be overwritten.
Consider prefixing (e.g. KISS_FFT_LVL_ERROR) or wrapping the block with #ifndef ERROR … #endif.
🤖 Prompt for AI Agents
In src/3rdparty/kissfft/kiss_fft_log.h around lines 12 to 16, the macros ERROR,
WARNING, INFO, and DEBUG conflict with common system header definitions. To fix
this, rename these macros by adding a unique prefix like KISS_FFT_LVL_ (e.g.,
KISS_FFT_LVL_ERROR) to avoid collisions, and update all references accordingly.
Alternatively, wrap the macro definitions with #ifndef checks to prevent
redefinition if they are already defined.
| #ifndef kiss_fft_log_h | ||
| #define kiss_fft_log_h | ||
|
|
There was a problem hiding this comment.
Missing <stdio.h> include breaks standalone compilation
fprintf is used by the logging macro but the header never pulls in <stdio.h>.
Any TU that includes kiss_fft_log.h before <stdio.h> (or does not include it at all) will fail to compile.
#ifndef kiss_fft_log_h
#define kiss_fft_log_h
+#include <stdio.h>🤖 Prompt for AI Agents
In src/3rdparty/kissfft/kiss_fft_log.h around lines 9 to 11, the header uses
fprintf in its logging macro but does not include <stdio.h>, causing compilation
errors if <stdio.h> is not included beforehand. Fix this by adding #include
<stdio.h> at the top of the header to ensure fprintf is declared and the header
can compile standalone.
| #if defined(NDEBUG) | ||
| # define KISS_FFT_LOG_MSG(severity, ...) ((void)0) | ||
| #else | ||
| # define KISS_FFT_LOG_MSG(severity, ...) \ | ||
| fprintf(stderr, "[" #severity "] " __FILE__ ":" TOSTRING(__LINE__) " "); \ | ||
| fprintf(stderr, __VA_ARGS__); \ | ||
| fprintf(stderr, "\n") | ||
| #endif |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Release builds drop all log output, even fatal errors
#if defined(NDEBUG) turns the whole logger into a no-op.
That kills KISS_FFT_ERROR() messages that could be invaluable in the field.
Keep ERROR (and maybe WARNING) active and mute only INFO/DEBUG:
#if defined(NDEBUG)
-# define KISS_FFT_LOG_MSG(severity, ...) ((void)0)
+# define KISS_FFT_LOG_MSG(severity, ...) \
+ do { \
+ if ((severity) <= WARNING) { \
+ fprintf(stderr, "[" #severity "] " __FILE__ ":" TOSTRING(__LINE__) " "); \
+ fprintf(stderr, __VA_ARGS__); \
+ fprintf(stderr, "\n"); \
+ } \
+ } while (0)📝 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.
| #if defined(NDEBUG) | |
| # define KISS_FFT_LOG_MSG(severity, ...) ((void)0) | |
| #else | |
| # define KISS_FFT_LOG_MSG(severity, ...) \ | |
| fprintf(stderr, "[" #severity "] " __FILE__ ":" TOSTRING(__LINE__) " "); \ | |
| fprintf(stderr, __VA_ARGS__); \ | |
| fprintf(stderr, "\n") | |
| #endif | |
| #if defined(NDEBUG) | |
| # define KISS_FFT_LOG_MSG(severity, ...) \ | |
| do { \ | |
| if ((severity) <= WARNING) { \ | |
| fprintf(stderr, "[" #severity "] " __FILE__ ":" TOSTRING(__LINE__) " "); \ | |
| fprintf(stderr, __VA_ARGS__); \ | |
| fprintf(stderr, "\n"); \ | |
| } \ | |
| } while (0) | |
| #else | |
| # define KISS_FFT_LOG_MSG(severity, ...) \ | |
| fprintf(stderr, "[" #severity "] " __FILE__ ":" TOSTRING(__LINE__) " "); \ | |
| fprintf(stderr, __VA_ARGS__); \ | |
| fprintf(stderr, "\n") | |
| #endif |
🤖 Prompt for AI Agents
In src/3rdparty/kissfft/kiss_fft_log.h around lines 20 to 27, the current code
disables all logging in release builds by making KISS_FFT_LOG_MSG a no-op when
NDEBUG is defined, which suppresses important ERROR logs. Modify the conditional
compilation so that ERROR (and optionally WARNING) logs remain active even in
release builds, while only INFO and DEBUG logs are disabled. This can be done by
checking the severity level inside the macro and allowing ERROR logs to always
print regardless of NDEBUG.
| jobs: | ||
| include: | ||
| - name: "build (make)" | ||
| script: | ||
| - make all | ||
| - make testall | ||
| - name: "build (cmake)" | ||
| script: | ||
| - mkdir build && cd build | ||
| - cmake .. | ||
| - make |
There was a problem hiding this comment.
Bad YAML indentation breaks Travis config.
YAMLlint already screamed; Travis will too. Two spaces are expected under script:.
- - name: "build (make)"
- script:
- - make all
- - make testall
- - name: "build (cmake)"
- script:
- - mkdir build && cd build
- - cmake ..
- - make
+ - name: "build (make)"
+ script:
+ - make all
+ - make testall
+ - name: "build (cmake)"
+ script:
+ - mkdir build && cd build
+ - cmake ..
+ - make📝 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.
| jobs: | |
| include: | |
| - name: "build (make)" | |
| script: | |
| - make all | |
| - make testall | |
| - name: "build (cmake)" | |
| script: | |
| - mkdir build && cd build | |
| - cmake .. | |
| - make | |
| jobs: | |
| include: | |
| - name: "build (make)" | |
| script: | |
| - make all | |
| - make testall | |
| - name: "build (cmake)" | |
| script: | |
| - mkdir build && cd build | |
| - cmake .. | |
| - make |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 21-21: wrong indentation: expected 8 but found 6
(indentation)
[error] 25-25: wrong indentation: expected 8 but found 6
(indentation)
🤖 Prompt for AI Agents
In src/3rdparty/kissfft/.travis.yml around lines 17 to 27, the indentation under
the script keys is incorrect, causing YAML parsing errors. Fix the indentation
by ensuring that each command under script is indented exactly two spaces
relative to the script key, aligning all script commands properly to conform to
YAML syntax.
| int main(void) | ||
| { | ||
| test1(); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
The test runs but asserts nothing.
Calling the inverse FFT without checking the result only catches segfaults.
Consider a simple round-trip sanity check so CI actually validates SIMD code paths.
🤖 Prompt for AI Agents
In src/3rdparty/kissfft/test/test_simd.c around lines 21 to 25, the main
function calls test1() but does not perform any assertions to verify
correctness. Modify the test to include a round-trip FFT followed by an inverse
FFT, then compare the output to the original input to confirm accuracy. Add
assertions that validate the results within an acceptable error margin to ensure
the SIMD code paths are properly tested in CI.
| if(CMAKE_SYSTEM_NAME MATCHES "^(Linux|kFreeBSD|GNU)$" AND NOT CMAKE_CROSSCOMPILING) | ||
| include(GNUInstallDirs) | ||
| endif() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
GNUInstallDirs only on Linux leaves CMAKE_INSTALL_LIBDIR undefined on macOS/BSD.
Call include(GNUInstallDirs) unconditionally; the module is cross-platform.
-if(CMAKE_SYSTEM_NAME MATCHES "^(Linux|kFreeBSD|GNU)$" AND NOT CMAKE_CROSSCOMPILING)
- include(GNUInstallDirs)
-endif()
+include(GNUInstallDirs)📝 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.
| if(CMAKE_SYSTEM_NAME MATCHES "^(Linux|kFreeBSD|GNU)$" AND NOT CMAKE_CROSSCOMPILING) | |
| include(GNUInstallDirs) | |
| endif() | |
| include(GNUInstallDirs) |
🤖 Prompt for AI Agents
In src/3rdparty/kissfft/CMakeLists.txt around lines 108 to 111, the
include(GNUInstallDirs) is conditional on Linux systems only, which causes
CMAKE_INSTALL_LIBDIR to be undefined on macOS and BSD. Remove the if condition
and call include(GNUInstallDirs) unconditionally to ensure CMAKE_INSTALL_LIBDIR
is defined on all platforms.
| %.c.o: %.c | ||
| $(CC) -Wall -fPIC \ | ||
| -o $@ \ | ||
| $(CFLAGS) $(TYPEFLAGS) -DKISS_FFT_BUILD \ | ||
| -c $< | ||
|
|
||
| # | ||
| # Target: "make all" | ||
| # | ||
|
|
||
| all: kfc.c.o kiss_fft.c.o kiss_fftnd.c.o kiss_fftndr.c.o kiss_fftr.c.o | ||
| ifneq ($(KISSFFT_STATIC), 1) | ||
| $(CC) $(KISSFFTLIB_FLAGS) -o $(KISSFFTLIB_NAME) $^ | ||
| ifneq ($(shell uname -s),Darwin) | ||
| ln -sf $(KISSFFTLIB_NAME) $(KISSFFTLIB_SONAME) | ||
| ln -sf $(KISSFFTLIB_NAME) $(KISSFFTLIB_SODEVELNAME) | ||
| endif | ||
| else | ||
| $(AR) crus $(KISSFFTLIB_NAME) $^ | ||
| endif | ||
| ifneq ($(KISSFFT_TOOLS), 0) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mark rule outputs to leverage parallelism and avoid accidental rebuilds.
Add explicit target/output relationships or use .PHONY for meta targets.
+.PHONY: all install clean testsingle testall asm🧰 Tools
🪛 checkmake (0.2.2)
[warning] 196-196: Target "all" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
In src/3rdparty/kissfft/Makefile around lines 186 to 206, the makefile lacks
explicit target-output declarations which can cause unnecessary rebuilds and
hinder parallel execution. To fix this, mark the actual file targets with their
corresponding output files to establish clear dependencies, and declare meta
targets like "all" as .PHONY to indicate they do not produce files. This will
help make recognize which targets are real files and which are just commands,
improving build efficiency and correctness.
| ifneq ($(shell uname -s),Darwin) | ||
| mkdir "$(ABS_LIBDIR)/pkgconfig" | ||
| sed \ | ||
| -e 's+@PKGCONFIG_KISSFFT_VERSION@+$(PKGCONFIG_KISSFFT_VERSION)+' \ | ||
| -e 's+@KISSFFT_OUTPUT_NAME@+$(PKGCONFIG_KISSFFT_OUTPUT_NAME)+' \ | ||
| -e 's+@PKG_KISSFFT_DEFS@+$(PKGCONFIG_PKG_KISSFFT_DEFS)+' \ | ||
| -e 's+@PKG_OPENMP@+$(PKGCONFIG_OPENMP)+' \ | ||
| -e 's+@PKGCONFIG_KISSFFT_PREFIX@+$(PKGCONFIG_KISSFFT_PREFIX)+' \ | ||
| -e 's+@PKGCONFIG_KISSFFT_INCLUDEDIR@+$(PKGCONFIG_KISSFFT_INCLUDEDIR)+' \ | ||
| -e 's+@PKGCONFIG_KISSFFT_LIBDIR@+$(PKGCONFIG_KISSFFT_LIBDIR)+' \ | ||
| -e 's+@PKGCONFIG_KISSFFT_PKGINCLUDEDIR@+$(PKGCONFIG_KISSFFT_PKGINCLUDEDIR)+' \ | ||
| kissfft.pc.in 1>"$(ABS_LIBDIR)/pkgconfig/$(KISSFFT_PKGCONFIG)" |
There was a problem hiding this comment.
mkdir without -p blows up on reinstall.
Re-running make install when the directory already exists currently fails.
- mkdir "$(ABS_LIBDIR)/pkgconfig"
+ mkdir -p "$(ABS_LIBDIR)/pkgconfig"📝 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.
| ifneq ($(shell uname -s),Darwin) | |
| mkdir "$(ABS_LIBDIR)/pkgconfig" | |
| sed \ | |
| -e 's+@PKGCONFIG_KISSFFT_VERSION@+$(PKGCONFIG_KISSFFT_VERSION)+' \ | |
| -e 's+@KISSFFT_OUTPUT_NAME@+$(PKGCONFIG_KISSFFT_OUTPUT_NAME)+' \ | |
| -e 's+@PKG_KISSFFT_DEFS@+$(PKGCONFIG_PKG_KISSFFT_DEFS)+' \ | |
| -e 's+@PKG_OPENMP@+$(PKGCONFIG_OPENMP)+' \ | |
| -e 's+@PKGCONFIG_KISSFFT_PREFIX@+$(PKGCONFIG_KISSFFT_PREFIX)+' \ | |
| -e 's+@PKGCONFIG_KISSFFT_INCLUDEDIR@+$(PKGCONFIG_KISSFFT_INCLUDEDIR)+' \ | |
| -e 's+@PKGCONFIG_KISSFFT_LIBDIR@+$(PKGCONFIG_KISSFFT_LIBDIR)+' \ | |
| -e 's+@PKGCONFIG_KISSFFT_PKGINCLUDEDIR@+$(PKGCONFIG_KISSFFT_PKGINCLUDEDIR)+' \ | |
| kissfft.pc.in 1>"$(ABS_LIBDIR)/pkgconfig/$(KISSFFT_PKGCONFIG)" | |
| ifneq ($(shell uname -s),Darwin) | |
| mkdir -p "$(ABS_LIBDIR)/pkgconfig" | |
| sed \ | |
| -e 's+@PKGCONFIG_KISSFFT_VERSION@+$(PKGCONFIG_KISSFFT_VERSION)+' \ | |
| -e 's+@KISSFFT_OUTPUT_NAME@+$(PKGCONFIG_KISSFFT_OUTPUT_NAME)+' \ | |
| -e 's+@PKG_KISSFFT_DEFS@+$(PKGCONFIG_PKG_KISSFFT_DEFS)+' \ | |
| -e 's+@PKG_OPENMP@+$(PKGCONFIG_OPENMP)+' \ | |
| -e 's+@PKGCONFIG_KISSFFT_PREFIX@+$(PKGCONFIG_KISSFFT_PREFIX)+' \ | |
| -e 's+@PKGCONFIG_KISSFFT_INCLUDEDIR@+$(PKGCONFIG_KISSFFT_INCLUDEDIR)+' \ | |
| -e 's+@PKGCONFIG_KISSFFT_LIBDIR@+$(PKGCONFIG_KISSFFT_LIBDIR)+' \ | |
| -e 's+@PKGCONFIG_KISSFFT_PKGINCLUDEDIR@+$(PKGCONFIG_KISSFFT_PKGINCLUDEDIR)+' \ | |
| kissfft.pc.in 1>"$(ABS_LIBDIR)/pkgconfig/$(KISSFFT_PKGCONFIG)" |
🤖 Prompt for AI Agents
In src/3rdparty/kissfft/Makefile around lines 229 to 240, the mkdir command is
used without the -p option, causing it to fail if the directory already exists
during reinstall. Modify the mkdir command to include the -p flag to ensure it
does not error out if the directory is already present.
| @@ -1,101 +1,188 @@ | |||
| # | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Declare phony targets
Add a .PHONY declaration for the main targets to satisfy checkmake and prevent name clashes:
.PHONY: all test clean testsse testcpp🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🤖 Prompt for AI Agents
In src/3rdparty/kissfft/test/Makefile at line 1, add a .PHONY declaration
listing the main targets such as all, test, clean, testsse, and testcpp. This
will prevent conflicts with files of the same name and satisfy the checkmake
tool. Insert the line '.PHONY: all test clean testsse testcpp' near the top of
the Makefile.
| ifneq ($(MAKECMDGOALS),clean) | ||
| PYTHON_INTERPRETER = $(shell python --version) | ||
| ifeq ($(PYTHON_INTERPRETER), ) | ||
| PYTHON_INTERPRETER = $(shell python2 --version) | ||
| ifeq ($(PYTHON_INTERPRETER), ) | ||
| PYTHON_INTERPRETER = $(shell python3 --version) | ||
| ifeq ($(PYTHON_INTERPRETER), ) | ||
| $(error ERROR: Can not find Python interpreter!) | ||
| else | ||
| PYTHON_INTERPRETER = "python3" | ||
| endif | ||
| else | ||
| PYTHON_INTERPRETER = "python2" | ||
| endif | ||
| else | ||
| PYTHON_INTERPRETER = "python" | ||
| endif | ||
| endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix Python interpreter detection
Using python --version captures a version string, not the executable path. Replace the block with a reliable lookup:
-PYTHON_INTERPRETER = $(shell python --version)
-ifeq ($(PYTHON_INTERPRETER), )
- PYTHON_INTERPRETER = $(shell python2 --version)
- ifeq ($(PYTHON_INTERPRETER), )
- PYTHON_INTERPRETER = $(shell python3 --version)
- ifeq ($(PYTHON_INTERPRETER), )
- $(error ERROR: Can not find Python interpreter!)
- else
- PYTHON_INTERPRETER = "python3"
- endif
- else
- PYTHON_INTERPRETER = "python2"
- endif
-else
- PYTHON_INTERPRETER = "python"
-endif
+PYTHON_INTERPRETER := $(shell { command -v python3 || command -v python2 || command -v python; } 2>/dev/null)
+ifeq ($(PYTHON_INTERPRETER),)
+ $(error ERROR: Cannot find a Python interpreter (python3, python2 or python).)
+endifThis ensures testkiss.py is invoked with a valid interpreter.
🤖 Prompt for AI Agents
In src/3rdparty/kissfft/test/Makefile lines 88 to 105, the current method uses
`python --version` which returns a version string instead of the executable
path, causing incorrect detection of the Python interpreter. Replace this block
with a reliable command like `which python` or `command -v python` to find the
actual interpreter path. Check for python, python2, and python3 in order, and
set PYTHON_INTERPRETER to the first valid executable found; if none are found,
raise an error. This ensures testkiss.py runs with a valid Python interpreter.
|
@Im-Beast Thank you for this. And sorry about the CodeRabbit spam. It's useful for my own PRs, not in cases like this it's useless. |
2 participants
Since mborgerding/kissfft@2e2747c
kiss_fftr.candkiss_fftr.hhave been moved from tools to the root directory.