[Clang-tidy] fix warning for Tscal used as loop counter#1424
Conversation
Summary of ChangesHello @Akos299, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the clang-tidy warning about using a floating-point type as a loop counter by replacing the for loop with a while loop. The change is functionally correct.
I've added one comment suggesting the removal of the old, commented-out code to improve maintainability.
Additionally, I noticed a few other places in the codebase that also use floating-point loop counters (e.g., in src/shammath/include/shammath/integrator.hpp:27 and src/tests/shammath/sphkernelsTests.cpp:226). It would be beneficial to address these in a follow-up pull request to ensure consistency and robustness across the project.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a clang-tidy warning about using a floating-point variable as a loop counter by converting a for loop to a while loop. While this silences the warning, it doesn't fix the underlying potential for floating-point precision issues. I've suggested a more robust approach using an integer-based loop, which avoids the accumulation of floating-point errors and makes the code more reliable.
8fb791d to
0033f6d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly resolves a clang-tidy warning about using a floating-point type as a loop counter. The change from a float-based for loop to an integer-based one is a standard and safe practice to avoid issues with floating-point precision. The implementation, which calculates the number of iterations beforehand using std::ceil, is robust and correctly maintains the original loop's logic. The addition of the <cmath> header is also appropriate. Overall, this is a good, focused fix that improves code quality.
Workflow reportworkflow report corresponding to commit 9a78a9b Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportNo relevant changes found. You should now go back to your normal life and enjoy a hopefully sunny day while waiting for the review. Doxygen diff with
|
…e#1424) warning: Variable 'x' with floating point type 'Tscal' should not be used as a loop counter [clang-analyzer-security.FloatLoopCounter] 85 | for (Tscal x = 0; x < Ker::Rkern; x += step) {...}; Replaced the foor loop by a while loop. Co-authored-by: David--Cléris Timothée <timothee.davidcleris@proton.me>
warning: Variable 'x' with floating point type 'Tscal' should not be used as a loop counter [clang-analyzer-security.FloatLoopCounter]
85 | for (Tscal x = 0; x < Ker::Rkern; x += step) {...};
Replaced the foor loop by a while loop.