Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
this would be great! |
…ishing # Conflicts: # .github/workflows/Release_AUR.yml # AGENTS.md
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request adds automated AUR packaging and publishing, updates the package to version 0.5.9, hardens fixed-point and input handling, and migrates driver tests to the Greatest framework with improved snapshot utilities. ChangesAUR release automation
Driver safety and test harness
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant ArchBuilder
participant AURRepository
GitHubActions->>ArchBuilder: Build and validate maccel-dkms
GitHubActions->>GitHubActions: Verify release, driver, CLI, and tag versions
GitHubActions->>AURRepository: Update package files and regenerate .SRCINFO
GitHubActions->>AURRepository: Commit and push changes
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Gnarus-G
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
.github/workflows/Release_AUR.yml (1)
20-21: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winOpt out of checkout credential persistence.
actions/checkoutdefaultspersist-credentialstotrue, which stores the checkout credential in the local repository after checkout. Addpersist-credentials: falseunless a later checkout-local step needs authenticated Git commands.
.github/workflows/Release_AUR.yml#L20-L21.github/workflows/Release_AUR.yml#L34-L35🤖 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 @.github/workflows/Release_AUR.yml around lines 20 - 21, Update both actions/checkout steps in .github/workflows/Release_AUR.yml at lines 20-21 and 34-35 to set persist-credentials to false, unless either checkout is followed by a step requiring authenticated Git commands.Source: Linters/SAST tools
driver/tests/guard_division.test.c (1)
136-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGroup these tests in a
SUITE.
maincallsRUN_TESTdirectly. Every other test file indriver/testsdefines aSUITEand callsRUN_SUITE. Without a suite, the Greatest-ssuite filter cannot select these tests, and the report omits the per-suite header and summary.♻️ Proposed refactor
+SUITE(division_guards) { + RUN_TEST(input_speed_time_zero_returns_zero); + RUN_TEST(faccelerate_input_dpi_zero_survives); + RUN_TEST(synchronous_motivity_one_survives); + RUN_TEST(faccelerate_time_zero_identity); + RUN_TEST(input_speed_negative_time_returns_zero); +} + GREATEST_MAIN_DEFS(); int main(int argc, char **argv) { TEST_MAIN_BEGIN(); - RUN_TEST(input_speed_time_zero_returns_zero); - RUN_TEST(faccelerate_input_dpi_zero_survives); - RUN_TEST(synchronous_motivity_one_survives); - RUN_TEST(faccelerate_time_zero_identity); - RUN_TEST(input_speed_negative_time_returns_zero); + RUN_SUITE(division_guards); GREATEST_MAIN_END(); }Based on the coding guideline "Adhere to existing project conventions, mimic surrounding code style".
🤖 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 `@driver/tests/guard_division.test.c` around lines 136 - 146, Define a Greatest SUITE for the division guard tests, move the existing RUN_TEST calls from main into that suite, and update main to invoke it with RUN_SUITE. Preserve the current test order and test names while matching the suite structure used by other files in driver/tests.Source: Coding guidelines
driver/tests/test_utils.h (1)
201-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider failing on a newly created snapshot when running in CI.
When the snapshot file does not exist,
assert_snapshotwrites the content, printsNEW, and returns0. The test then passes. If a snapshot file is missing or was not committed, CI reports success and the assertion provides no coverage.Gate the auto-create behavior on an environment variable so CI fails instead.
♻️ Proposed refactor
} else { fprintf(snapshot_file, "%s", content); printf("NEW %s\n", filename); + fclose(snapshot_file); + if (getenv("CI") != NULL) { + fprintf(stderr, "snapshot did not exist: %s\n", filename); + return 1; + } + return 0; } fclose(snapshot_file); return 0;🤖 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 `@driver/tests/test_utils.h` around lines 201 - 207, Update assert_snapshot’s missing-file branch to check a designated environment variable before auto-creating the snapshot; allow creation and return success only when explicitly enabled, otherwise report the missing snapshot as a failure and return a nonzero result. Preserve the existing write and NEW output for local runs.
🤖 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 @.github/workflows/Release_AUR.yml:
- Line 15: Replace the archlinux:latest container references in
.github/workflows/Release_AUR.yml at lines 15-15 and 32-32 with the same
approved immutable digest-pinned Arch image, updating it through the reviewed
dependency-update process.
In `@driver/input_handler.h`:
- Around line 116-140: Move each rotation injection debug call into its
corresponding syn_pos success branch: place the REL_X log after the X event is
inserted and the REL_Y log after the Y event is inserted. Keep the existing
guards and insertion logic unchanged so no injection is reported when syn_pos is
NULL.
In `@driver/tests/atofp.test.c`:
- Around line 23-31: Update the elapsed-time calculation in the benchmark loop
around clock_gettime to include both tv_sec and tv_nsec when computing each
interval, so timings remain correct across second boundaries. Keep the existing
sum accumulation and average output unchanged.
In `@driver/tests/div128_s64.test.c`:
- Around line 26-50: Move the divides_zero_divisor test definition into the
`#else` branch corresponding to `#if` FIXEDPT_BITS == 32, alongside its test
registration. Keep the test body unchanged and ensure div128_s64_s64 is
referenced only in the non-32-bit build.
In `@driver/tests/fp_to_str.test.c`:
- Around line 12-18: Update the fp_to_str_test macro to derive the snapshot
filename using the kernel/build-supported basename approach from __FILE__
instead of relying on the compiler-specific __FILE_NAME__ extension; preserve
the existing snapshot comparison and assertion behavior.
In `@driver/tests/guard_division.test.c`:
- Around line 118-122: Update the ASSERT_EQ_FMTm call in
faccelerate_time_zero_identity to use a message describing the expected identity
behavior, rather than “should not crash.” Keep the assertion values and
formatting unchanged.
In `@driver/tests/input_speed.test.c`:
- Around line 12-16: Update the two fptoa calls in input_speed test output to
pass the existing fixed-point values dx and dy instead of the double inputs x
and y, preserving the intended converted-value diagnostics.
In `@packaging/aur/PKGBUILD`:
- Line 7: Update packaging/aur/PKGBUILD’s _driverver to 0.5.9 so it matches the
root PKGBUILD pkgver. In .github/workflows/Release_AUR.yml, parse _driverver
from the AUR PKGBUILD and pkgver from the root PKGBUILD, then fail the workflow
when the values differ.
---
Nitpick comments:
In @.github/workflows/Release_AUR.yml:
- Around line 20-21: Update both actions/checkout steps in
.github/workflows/Release_AUR.yml at lines 20-21 and 34-35 to set
persist-credentials to false, unless either checkout is followed by a step
requiring authenticated Git commands.
In `@driver/tests/guard_division.test.c`:
- Around line 136-146: Define a Greatest SUITE for the division guard tests,
move the existing RUN_TEST calls from main into that suite, and update main to
invoke it with RUN_SUITE. Preserve the current test order and test names while
matching the suite structure used by other files in driver/tests.
In `@driver/tests/test_utils.h`:
- Around line 201-207: Update assert_snapshot’s missing-file branch to check a
designated environment variable before auto-creating the snapshot; allow
creation and return success only when explicitly enabled, otherwise report the
missing snapshot as a failure and return a nonzero result. Preserve the existing
write and NEW output for local runs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 82f8e174-6a01-4e5c-8af1-e96aa349de4c
📒 Files selected for processing (19)
.github/workflows/Release_AUR.ymlAGENTS.mdPKGBUILDdriver/fixedptc.hdriver/input_handler.hdriver/speed.hdriver/tests/accel.test.cdriver/tests/atofp.test.cdriver/tests/div128_s64.test.cdriver/tests/fp_to_str.test.cdriver/tests/guard_division.test.cdriver/tests/input_speed.test.cdriver/tests/test_utils.hdriver/tests/vendor/greatest.hpackaging/aur/LICENSEpackaging/aur/PKGBUILDpackaging/aur/maccel.installtui/src/param_input.rstui/src/screen.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
🧹 Nitpick comments (3)
.github/workflows/Release_AUR.yml (1)
20-21: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winOpt out of checkout credential persistence.
actions/checkoutdefaultspersist-credentialstotrue, which stores the checkout credential in the local repository after checkout. Addpersist-credentials: falseunless a later checkout-local step needs authenticated Git commands.
.github/workflows/Release_AUR.yml#L20-L21.github/workflows/Release_AUR.yml#L34-L35🤖 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 @.github/workflows/Release_AUR.yml around lines 20 - 21, Update both actions/checkout steps in .github/workflows/Release_AUR.yml at lines 20-21 and 34-35 to set persist-credentials to false, unless either checkout is followed by a step requiring authenticated Git commands.Source: Linters/SAST tools
driver/tests/guard_division.test.c (1)
136-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGroup these tests in a
SUITE.
maincallsRUN_TESTdirectly. Every other test file indriver/testsdefines aSUITEand callsRUN_SUITE. Without a suite, the Greatest-ssuite filter cannot select these tests, and the report omits the per-suite header and summary.♻️ Proposed refactor
+SUITE(division_guards) { + RUN_TEST(input_speed_time_zero_returns_zero); + RUN_TEST(faccelerate_input_dpi_zero_survives); + RUN_TEST(synchronous_motivity_one_survives); + RUN_TEST(faccelerate_time_zero_identity); + RUN_TEST(input_speed_negative_time_returns_zero); +} + GREATEST_MAIN_DEFS(); int main(int argc, char **argv) { TEST_MAIN_BEGIN(); - RUN_TEST(input_speed_time_zero_returns_zero); - RUN_TEST(faccelerate_input_dpi_zero_survives); - RUN_TEST(synchronous_motivity_one_survives); - RUN_TEST(faccelerate_time_zero_identity); - RUN_TEST(input_speed_negative_time_returns_zero); + RUN_SUITE(division_guards); GREATEST_MAIN_END(); }Based on the coding guideline "Adhere to existing project conventions, mimic surrounding code style".
🤖 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 `@driver/tests/guard_division.test.c` around lines 136 - 146, Define a Greatest SUITE for the division guard tests, move the existing RUN_TEST calls from main into that suite, and update main to invoke it with RUN_SUITE. Preserve the current test order and test names while matching the suite structure used by other files in driver/tests.Source: Coding guidelines
driver/tests/test_utils.h (1)
201-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider failing on a newly created snapshot when running in CI.
When the snapshot file does not exist,
assert_snapshotwrites the content, printsNEW, and returns0. The test then passes. If a snapshot file is missing or was not committed, CI reports success and the assertion provides no coverage.Gate the auto-create behavior on an environment variable so CI fails instead.
♻️ Proposed refactor
} else { fprintf(snapshot_file, "%s", content); printf("NEW %s\n", filename); + fclose(snapshot_file); + if (getenv("CI") != NULL) { + fprintf(stderr, "snapshot did not exist: %s\n", filename); + return 1; + } + return 0; } fclose(snapshot_file); return 0;🤖 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 `@driver/tests/test_utils.h` around lines 201 - 207, Update assert_snapshot’s missing-file branch to check a designated environment variable before auto-creating the snapshot; allow creation and return success only when explicitly enabled, otherwise report the missing snapshot as a failure and return a nonzero result. Preserve the existing write and NEW output for local runs.
🤖 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 @.github/workflows/Release_AUR.yml:
- Line 15: Replace the archlinux:latest container references in
.github/workflows/Release_AUR.yml at lines 15-15 and 32-32 with the same
approved immutable digest-pinned Arch image, updating it through the reviewed
dependency-update process.
In `@driver/input_handler.h`:
- Around line 116-140: Move each rotation injection debug call into its
corresponding syn_pos success branch: place the REL_X log after the X event is
inserted and the REL_Y log after the Y event is inserted. Keep the existing
guards and insertion logic unchanged so no injection is reported when syn_pos is
NULL.
In `@driver/tests/atofp.test.c`:
- Around line 23-31: Update the elapsed-time calculation in the benchmark loop
around clock_gettime to include both tv_sec and tv_nsec when computing each
interval, so timings remain correct across second boundaries. Keep the existing
sum accumulation and average output unchanged.
In `@driver/tests/div128_s64.test.c`:
- Around line 26-50: Move the divides_zero_divisor test definition into the
`#else` branch corresponding to `#if` FIXEDPT_BITS == 32, alongside its test
registration. Keep the test body unchanged and ensure div128_s64_s64 is
referenced only in the non-32-bit build.
In `@driver/tests/fp_to_str.test.c`:
- Around line 12-18: Update the fp_to_str_test macro to derive the snapshot
filename using the kernel/build-supported basename approach from __FILE__
instead of relying on the compiler-specific __FILE_NAME__ extension; preserve
the existing snapshot comparison and assertion behavior.
In `@driver/tests/guard_division.test.c`:
- Around line 118-122: Update the ASSERT_EQ_FMTm call in
faccelerate_time_zero_identity to use a message describing the expected identity
behavior, rather than “should not crash.” Keep the assertion values and
formatting unchanged.
In `@driver/tests/input_speed.test.c`:
- Around line 12-16: Update the two fptoa calls in input_speed test output to
pass the existing fixed-point values dx and dy instead of the double inputs x
and y, preserving the intended converted-value diagnostics.
In `@packaging/aur/PKGBUILD`:
- Line 7: Update packaging/aur/PKGBUILD’s _driverver to 0.5.9 so it matches the
root PKGBUILD pkgver. In .github/workflows/Release_AUR.yml, parse _driverver
from the AUR PKGBUILD and pkgver from the root PKGBUILD, then fail the workflow
when the values differ.
---
Nitpick comments:
In @.github/workflows/Release_AUR.yml:
- Around line 20-21: Update both actions/checkout steps in
.github/workflows/Release_AUR.yml at lines 20-21 and 34-35 to set
persist-credentials to false, unless either checkout is followed by a step
requiring authenticated Git commands.
In `@driver/tests/guard_division.test.c`:
- Around line 136-146: Define a Greatest SUITE for the division guard tests,
move the existing RUN_TEST calls from main into that suite, and update main to
invoke it with RUN_SUITE. Preserve the current test order and test names while
matching the suite structure used by other files in driver/tests.
In `@driver/tests/test_utils.h`:
- Around line 201-207: Update assert_snapshot’s missing-file branch to check a
designated environment variable before auto-creating the snapshot; allow
creation and return success only when explicitly enabled, otherwise report the
missing snapshot as a failure and return a nonzero result. Preserve the existing
write and NEW output for local runs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 82f8e174-6a01-4e5c-8af1-e96aa349de4c
📒 Files selected for processing (19)
.github/workflows/Release_AUR.ymlAGENTS.mdPKGBUILDdriver/fixedptc.hdriver/input_handler.hdriver/speed.hdriver/tests/accel.test.cdriver/tests/atofp.test.cdriver/tests/div128_s64.test.cdriver/tests/fp_to_str.test.cdriver/tests/guard_division.test.cdriver/tests/input_speed.test.cdriver/tests/test_utils.hdriver/tests/vendor/greatest.hpackaging/aur/LICENSEpackaging/aur/PKGBUILDpackaging/aur/maccel.installtui/src/param_input.rstui/src/screen.rs
🛑 Comments failed to post (6)
driver/input_handler.h (1)
116-140: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Move the "injected" debug log inside the success branch.
For both the X and Y blocks,
dbg("rotation: injected synthetic REL_X/REL_Y = ...")sits outside theif (syn_pos)check. Whensyn_posisNULL(noSYN_REPORTfound), no memmove or field write happens, but the log still claims the value was injected. Move the log inside theif (syn_pos)block so it reflects the actual outcome.🐛 Proposed fix
if (injected_x && synthetic_x_val != NONE_EVENT_VALUE && _count < max) { if (syn_pos) { /* Shift SYN_REPORT and everything after it forward by one */ memmove(syn_pos + 1, syn_pos, (end - syn_pos) * sizeof(*syn_pos)); syn_pos->type = EV_REL; syn_pos->code = REL_X; syn_pos->value = synthetic_x_val; syn_pos++; end++; _count++; + dbg("rotation: injected synthetic REL_X = %d", synthetic_x_val); } - dbg("rotation: injected synthetic REL_X = %d", synthetic_x_val); } if (injected_y && synthetic_y_val != NONE_EVENT_VALUE && _count < max) { if (syn_pos) { memmove(syn_pos + 1, syn_pos, (end - syn_pos) * sizeof(*syn_pos)); syn_pos->type = EV_REL; syn_pos->code = REL_Y; syn_pos->value = synthetic_y_val; end++; _count++; + dbg("rotation: injected synthetic REL_Y = %d", synthetic_y_val); } - dbg("rotation: injected synthetic REL_Y = %d", synthetic_y_val); }📝 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 (injected_x && synthetic_x_val != NONE_EVENT_VALUE && _count < max) { if (syn_pos) { /* Shift SYN_REPORT and everything after it forward by one */ memmove(syn_pos + 1, syn_pos, (end - syn_pos) * sizeof(*syn_pos)); syn_pos->type = EV_REL; syn_pos->code = REL_X; syn_pos->value = synthetic_x_val; syn_pos++; end++; _count++; dbg("rotation: injected synthetic REL_X = %d", synthetic_x_val); } } if (injected_y && synthetic_y_val != NONE_EVENT_VALUE && _count < max) { if (syn_pos) { memmove(syn_pos + 1, syn_pos, (end - syn_pos) * sizeof(*syn_pos)); syn_pos->type = EV_REL; syn_pos->code = REL_Y; syn_pos->value = synthetic_y_val; end++; _count++; dbg("rotation: injected synthetic REL_Y = %d", synthetic_y_val); } }🤖 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 `@driver/input_handler.h` around lines 116 - 140, Move each rotation injection debug call into its corresponding syn_pos success branch: place the REL_X log after the X event is inserted and the REL_Y log after the Y event is inserted. Keep the existing guards and insertion logic unchanged so no injection is reported when syn_pos is NULL.driver/tests/atofp.test.c (1)
23-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the elapsed-time arithmetic in the benchmark.
Line 28 subtracts only
tv_nsecand ignorestv_sec. When the interval crosses a second boundary,end.tv_nsec - begin.tv_nsecis negative. The accumulatedsumthen under-reports, and the printed average is wrong.Include
tv_secin the difference.🐛 Proposed fix
clock_gettime(CLOCK_MONOTONIC_RAW, &end); - sum += (double)(end.tv_nsec - begin.tv_nsec); + sum += (double)(end.tv_sec - begin.tv_sec) * 1e9 + + (double)(end.tv_nsec - begin.tv_nsec);📝 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.for (int i = 0; i < iterations; i++) { struct timespec begin, end; clock_gettime(CLOCK_MONOTONIC_RAW, &begin); atofp("1826512586328"); clock_gettime(CLOCK_MONOTONIC_RAW, &end); sum += (double)(end.tv_sec - begin.tv_sec) * 1e9 + (double)(end.tv_nsec - begin.tv_nsec); } printf("\n Average conversion: %fns\n", sum / iterations);🧰 Tools
🪛 Clang (14.0.6)
[error] 25-25: use of undeclared identifier 'CLOCK_MONOTONIC_RAW'
(clang-diagnostic-error)
[error] 27-27: use of undeclared identifier 'CLOCK_MONOTONIC_RAW'
(clang-diagnostic-error)
🤖 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 `@driver/tests/atofp.test.c` around lines 23 - 31, Update the elapsed-time calculation in the benchmark loop around clock_gettime to include both tv_sec and tv_nsec when computing each interval, so timings remain correct across second boundaries. Keep the existing sum accumulation and average output unchanged.driver/tests/div128_s64.test.c (1)
26-50: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Move
divides_zero_divisorinside theFIXEDPT_BITS != 32branch.
divides_zero_divisoris defined unconditionally at Line 26, but the suite registers it only in the#elsebranch at Line 71. Two problems follow on a 32-bit build:
- The
staticfunction is defined and never used. The compiler emits-Wunused-function. If the test build uses-Werror, the build fails.- The function body calls
div128_s64_s64unconditionally. The surrounding#if FIXEDPT_BITS == 32guard exists to keep the 64-bit division helper out of 32-bit builds, so this call defeats the guard.Move the definition into the same
#elsebranch that registers it.🐛 Proposed fix
-/* - * A zero divisor currently raises SIGFPE in the raw idivq helper. The guard - * must make division by zero *return* a defined value instead of trapping, so - * run it in a forked child and require a clean exit (red now, green after the - * guard). This keeps the failure an assertion instead of killing the suite. - */ -TEST divides_zero_divisor(void) { - pid_t pid = fork(); - ... -} - `#if` FIXEDPT_BITS == 32 TEST division_is_not_supported(void) { SKIP(); } `#else` +/* + * A zero divisor currently raises SIGFPE in the raw idivq helper. The guard + * must make division by zero *return* a defined value instead of trapping, so + * run it in a forked child and require a clean exit (red now, green after the + * guard). This keeps the failure an assertion instead of killing the suite. + */ +TEST divides_zero_divisor(void) { + /* unchanged body */ +} + division_test(divides_positive_values, 57, 5.5);📝 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` FIXEDPT_BITS == 32 TEST division_is_not_supported(void) { SKIP(); } `#else` /* * A zero divisor currently raises SIGFPE in the raw idivq helper. The guard * must make division by zero *return* a defined value instead of trapping, so * run it in a forked child and require a clean exit (red now, green after the * guard). This keeps the failure an assertion instead of killing the suite. */ TEST divides_zero_divisor(void) { pid_t pid = fork(); if (pid == -1) { perror("fork"); FAILm("fork failed"); } if (pid == 0) { freopen("/dev/null", "w", stdout); fpt quotient = div128_s64_s64(fpt_rconst(127), 0); if (quotient != 0) _exit(2); _exit(0); } int status; pid_t result; do { result = waitpid(pid, &status, 0); } while (result == -1 && errno == EINTR); if (result == -1) { perror("waitpid"); FAILm("waitpid failed"); } ASSERT_EQ_FMTm("division by zero should return zero", 0, status, "%d"); PASS(); } division_test(divides_positive_values, 57, 5.5);🤖 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 `@driver/tests/div128_s64.test.c` around lines 26 - 50, Move the divides_zero_divisor test definition into the `#else` branch corresponding to `#if` FIXEDPT_BITS == 32, alongside its test registration. Keep the test body unchanged and ensure div128_s64_s64 is referenced only in the non-32-bit build.driver/tests/fp_to_str.test.c (1)
12-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
GCC version that added __FILE_NAME__ predefined macro💡 Result:
The FILE_NAME predefined macro was added to GCC in version 12 [1][2]. This macro expands to the basename of the current input file as a C string constant [3][4]. It serves as a space-saving alternative to the FILE macro, which typically provides the full path to the file [1].
Citations:
- 1: mesonbuild/meson#12477
- 2: https://gcc.gnu.org/onlinedocs/gcc-12.1.0/cpp.pdf.gz
- 3: https://snapshots.sourceware.org/gcc/docs/latest/cpp/Common-Predefined-Macros.html
- 4: https://splichal.eu/scripts/sphinx/cpp/_build/html/macros/predefined-macros.html
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Files mentioning __FILE_NAME__ or compiler requirements ==" rg -n "__FILE_NAME__|GCC|MinGW|compiler|cc|cflags|cppcheck|compiler_flags|CMAKE_C_COMPILER|GNU_VERSION|cc_version" -S . --glob '!node_modules/**' --glob '!build/**' --glob '!dist/**' || true echo echo "== Candidate project config files ==" git ls-files | rg '(^|/)(Makefile|CMakeLists\.txt|meson\.build|configure\.(ac|in)|.github/workflows/.*\.ya?ml|configure\.py|tools/.*|.*\.mk|pyproject\.toml|requirements.*\.txt)' || true echo echo "== driver/tests/fp_to_str.test.c ==" cat -n driver/tests/fp_to_str.test.c echo echo "== nearby driver test files with similar TEST macro ==" rg -n "`#define` TEST|__FILE_NAME__|assert_snapshot|assert_string_value" driver || trueRepository: Gnarus-G/maccel
Length of output: 50371
Use a supported
__FILE__basename expression in the snapshot macro.
__FILE_NAME__is a GCC 12 / Clang 9 extension, but the driver tests use plaingccwithout a minimum version or clang fallback. With older GCC,__FILE_NAME__is undefined anddriver/tests/fp_to_str.test.cwill fail during macro expansion. Use the kernel/build-supported basename approach, e.g.basename(__FILE__), or document/update the supported compiler range.🤖 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 `@driver/tests/fp_to_str.test.c` around lines 12 - 18, Update the fp_to_str_test macro to derive the snapshot filename using the kernel/build-supported basename approach from __FILE__ instead of relying on the compiler-specific __FILE_NAME__ extension; preserve the existing snapshot comparison and assertion behavior.Source: Linters/SAST tools
driver/tests/guard_division.test.c (1)
118-122: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the assertion message.
The child at Lines 109-116 exits with code 2 when
xorychange. The assertion therefore verifies the identity expectation, not only survival. The message "should not crash" does not describe that check. Every other test in this file uses a message that names the expectation.✏️ Proposed fix
TEST faccelerate_time_zero_identity(void) { int status = run_child_exit(child_faccelerate_time_zero_identity); - ASSERT_EQ_FMTm("should not crash", 0, status, "%d"); + ASSERT_EQ_FMTm("f_accelerate(time=0) should leave x and y unchanged", 0, + status, "%d"); PASS(); }📝 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.TEST faccelerate_time_zero_identity(void) { int status = run_child_exit(child_faccelerate_time_zero_identity); ASSERT_EQ_FMTm("f_accelerate(time=0) should leave x and y unchanged", 0, status, "%d"); PASS(); }🤖 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 `@driver/tests/guard_division.test.c` around lines 118 - 122, Update the ASSERT_EQ_FMTm call in faccelerate_time_zero_identity to use a message describing the expected identity behavior, rather than “should not crash.” Keep the assertion values and formatting unchanged.driver/tests/input_speed.test.c (1)
12-16: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pass the fixed-point values to
fptoa, not the doubles.Lines 13 and 14 call
fptoa(x)andfptoa(y).xandyaredouble.fptoatakes anfpt, which is an integer type. The compiler converts the double value by truncation, sofptoareceives1instead of the fixed-point representationfpt_rconst(1). The debug output reports a value that is not the converted input, which defeats the purpose of the two lines. The converted values are already available asdxanddy.🐛 Proposed fix
dbg("in (%f, %f)", x, y); - dbg("in: x (fpt conversion) %s", fptoa(x)); - dbg("in: y (fpt conversion) %s", fptoa(y)); + dbg("in: x (fpt conversion) %s", fptoa(dx)); + dbg("in: y (fpt conversion) %s", fptoa(dy)); dbg("(%f, %f) dt = %f -> %f\n", x, y, t, result);📝 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.dbg("in (%f, %f)", x, y); dbg("in: x (fpt conversion) %s", fptoa(dx)); dbg("in: y (fpt conversion) %s", fptoa(dy)); dbg("(%f, %f) dt = %f -> %f\n", x, y, t, result); sprintf(content, "(sqrt(%f, %f) / %f) = %f\n", x, y, t, result);🧰 Tools
🪛 ast-grep (0.45.0)
[error] 15-15: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: sprintf(content, "(sqrt(%f, %f) / %f) = %f\n", x, y, t, result)
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').(dangerous-buffer-functions-c)
🤖 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 `@driver/tests/input_speed.test.c` around lines 12 - 16, Update the two fptoa calls in input_speed test output to pass the existing fixed-point values dx and dy instead of the double inputs x and y, preserving the intended converted-value diagnostics.
Automate and publish the AUR package
2 participants
Summary
maccel-dkmsAUR packageVerification
makepkgpackage buildmakepkg --printsrcinfo --dir packaging/auractionlint .github/workflows/Release_AUR.ymlConfiguration required
Repository secrets
AUR_USERNAME,AUR_EMAIL, andAUR_SSH_KEYmust be configured before publication.Fixes #32
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores