Add tasks to run C++ static analysis using clang-tidy; Separate CMake project config into its own task. #18
Conversation
WalkthroughThe pull request introduces several enhancements across multiple files. A new section is added to Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
| cmd: |- | ||
| . "{{.G_LINT_VENV_DIR}}/bin/activate" | ||
| find "{{.G_SRC_CLP_FFI_JS_DIR}}" \ | ||
| -type f \ | ||
| \( -iname "*.cpp" -o -iname "*.h" -o -iname "*.hpp" \) \ | ||
| -print0 | \ | ||
| xargs -0 clang-format {{.FLAGS}} -Werror |
There was a problem hiding this comment.
Unrelated to the PR, but this is simply to reformat from cmds into cmd.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
lint-requirements.txt (2)
1-2: LGTM: Version lock for clang-format is well-explained.The decision to lock clang-format to version 18.x is clearly justified. This approach prevents unexpected formatting changes while acknowledging the need for future updates.
Consider creating a future task to upgrade the codebase to meet v19's formatting standards. This will ensure the project stays up-to-date with the latest best practices in code formatting.
1-4: Overall, the changes to lint-requirements.txt are appropriate and align with the PR objectives.The addition of clang-tidy for static analysis is a positive step towards improving code quality. The file structure remains clear and concise.
For future consideration:
- Plan for upgrading clang-format to v19 to align with clang-tidy's version.
- Consider adding comments for yamllint and clang-tidy similar to the one for clang-format, explaining any version constraints or future upgrade plans.
README.md (2)
63-63: Fix typo in file type reference.There's a small typo in the explanation.
Please apply the following change:
-if you only changed C+ files, you don't need to run the YAML linting checks) using one of the tasks +if you only changed C++ files, you don't need to run the YAML linting checks) using one of the tasks
61-75: Great addition to the README!These changes effectively document the new linting tasks, particularly the C++ static analysis using clang-tidy. The clear structure and explanations will be very helpful for contributors. The content aligns well with the PR objectives and enhances the project's documentation.
A suggestion for future improvements:
- Consider adding a brief note about the plan to parallelize clang-tidy's checks in the future, as mentioned in the PR description. This could be added as a comment or a "Future Plans" section.
lint-tasks.yml (5)
23-30: Well-structured separation of linting concerns.The restructuring of
cpp-checkandcpp-fixtasks into separate formatting and static analysis subtasks is excellent. This separation aligns perfectly with the PR objectives and provides more granular control over the linting process. It also sets a good foundation for future parallelization of clang-tidy checks.One minor suggestion to consider:
Consider adding comments to briefly explain the purpose of each subtask, especially for newcomers to the project. For example:
cpp-check: cmds: - task: "cpp-format-check" # Check C++ code formatting - task: "cpp-static-check" # Run static analysis on C++ code cpp-fix: cmds: - task: "cpp-format-fix" # Fix C++ code formatting issues - task: "cpp-static-fix" # Apply fixes from static analysis (currently an alias)
Line range hint
32-50: Excellent implementation of formatting tasks with shared source definitions.The new
cpp-format-checkandcpp-format-fixtasks are well-structured and make good use of YAML anchors to share source file definitions. This approach reduces duplication and ensures consistency across tasks. The separation of check and fix tasks provides flexibility in CI/CD pipelines and local development workflows.A minor suggestion for improvement:
Consider adding a comment explaining the purpose of the
FLAGSvariable in each task. For example:cpp-format-check: # ... (existing code) cmds: - task: "clang-format" vars: FLAGS: "--dry-run" # Check formatting without making changes cpp-format-fix: # ... (existing code) cmds: - task: "clang-format" vars: FLAGS: "-i" # Apply formatting changes in-placeThis addition would enhance clarity for developers who might be less familiar with clang-format options.
52-66: Well-implemented static analysis task with clear explanations.The
cpp-static-checktask is well-structured and aligns perfectly with the PR objectives. The decision to alias it tocpp-static-fixwhile not implementing automatic fixes is well-explained in the comments. The comprehensive list of source files ensures thorough coverage of the codebase during static analysis.A suggestion for future improvement:
As the project evolves, consider implementing a mechanism to selectively apply safe automatic fixes from clang-tidy. This could be done by:
- Identifying a subset of clang-tidy checks that consistently produce safe fixes.
- Implementing a separate task for applying only these safe fixes.
- Updating the
cpp-static-fixtask to use this new selective fix mechanism.This approach would maintain the current cautious stance while gradually introducing the benefits of automatic fixes where appropriate.
88-94: Efficient implementation of the clang-format task.The modifications to the
clang-formattask are well-done. The change fromcmdstocmdsimplifies the task structure, and the use offindwithxargsis an efficient approach for processing multiple files. The inclusion of the-Werrorflag ensures strict adherence to formatting standards.A suggestion for improved robustness:
Consider adding the
-print0option tofindand the-0option toxargsto handle filenames with spaces or special characters correctly. For example:cmd: |- . "{{.G_LINT_VENV_DIR}}/bin/activate" find "{{.G_SRC_CLP_FFI_JS_DIR}}" \ -type f \ \( -iname "*.cpp" -o -iname "*.h" -o -iname "*.hpp" \) \ -print0 | \ xargs -0 clang-format {{.FLAGS}} -WerrorThis change ensures that the task works correctly even if there are files with spaces or special characters in their names.
96-115: Comprehensive clang-tidy task with excellent Emscripten integration.The new
clang-tidytask is well-implemented and shows careful consideration of the project's specific needs. The inclusion of Emscripten's compiler flags ensures that the static analysis is accurate for your project's context. The use offindandxargsis consistent with theclang-formattask, which is great for maintainability.Two suggestions for improvement:
Similar to the
clang-formattask, consider using-print0withfindand-0withxargsto handle filenames with spaces or special characters:find "{{.G_SRC_CLP_FFI_JS_DIR}}" \ -type f \ \( -iname "*.cpp" -o -iname "*.h" -o -iname "*.hpp" \) \ -print0 | \ xargs -0 clang-tidy {{.FLAGS}} $EXTRA_ARGSConsider adding a comment explaining the purpose of the
EXTRA_ARGSvariable and why Emscripten's flags are necessary. This would help other developers understand the task's complexity.As the project grows, you might want to consider creating a separate configuration file for Emscripten-specific clang-tidy settings. This could help manage the complexity of your static analysis setup and make it easier to adjust settings specific to Emscripten-compiled code.
Taskfile.yml (1)
59-69: Well-structured new task for CMake configuration.The new
config-cmake-projecttask is a great addition that improves the organization of the build process. It correctly encapsulates the CMake configuration and depends on theemsdktask.Consider breaking the long CMake command into multiple lines for improved readability:
cmd: >- cmake -G "Unix Makefiles" -DCMAKE_TOOLCHAIN_FILE="{{.G_EMSDK_DIR}}/upstream/emscripten/cmake/Modules/Platform/Emscripten.cmake" -S "{{.ROOT_DIR}}" -B "{{.G_CLP_FFI_JS_BUILD_DIR}}"This format maintains the same functionality while making it easier to read and modify in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- README.md (1 hunks)
- Taskfile.yml (2 hunks)
- lint-requirements.txt (1 hunks)
- lint-tasks.yml (4 hunks)
🔇 Additional comments (10)
lint-requirements.txt (2)
4-4: LGTM: yamllint dependency remains unchanged.The yamllint dependency specification is appropriate, allowing for future updates while ensuring a minimum version for compatibility.
3-3: Verify version compatibility between clang-format and clang-tidy.The addition of clang-tidy aligns well with the PR objective of introducing C++ static analysis. The flexible version specification (>=19.1.0) is good for future-proofing.
However, there's a potential version mismatch between clang-format (v18.x) and clang-tidy (>=19.1.0). Please verify that these different versions are compatible and won't cause conflicts in the CI pipeline or developer environments. Consider running the following commands to check compatibility:
README.md (2)
61-65: LGTM! Clear introduction to specific linters.The introduction effectively explains the purpose and benefits of running specific linters. It provides a good context for the table that follows.
66-75: Excellent table of linting tasks!The table provides a comprehensive overview of the available linting tasks for both C++ and YAML. The separation of C++ tasks into formatting and static analysis is particularly helpful and aligns well with the PR objectives.
lint-tasks.yml (3)
19-19: Excellent optimization for thecpp-configstask.Adding
run: "once"to thecpp-configstask is a smart optimization. This ensures that the configuration is set up only once, which is typically sufficient for most linting sessions. This change can help improve the overall efficiency of the linting process, especially when running multiple linting tasks in succession.
127-127: Appropriate optimization for thevenvtask.Adding
run: "once"to thevenvtask is a sensible optimization. This ensures that the virtual environment is set up only once per session, which is typically sufficient and can significantly improve the efficiency of subsequent task runs. This change is consistent with the similar optimization applied to thecpp-configstask, showing a good overall approach to task optimization in the file.
Line range hint
1-143: Excellent overhaul of C++ linting tasks with improved structure and efficiency.This update to
lint-tasks.ymlsignificantly enhances the C++ linting capabilities of the project. The changes align perfectly with the PR objectives, introducing clang-tidy for static analysis and effectively separating formatting and static analysis tasks. Key improvements include:
- Clear separation of concerns between formatting and static analysis tasks.
- Efficient task structuring with the use of YAML anchors and aliases.
- Comprehensive coverage of source files for both formatting and static analysis.
- Thoughtful integration of Emscripten-specific settings for accurate static analysis.
- Performance optimizations through the use of
run: "once"for appropriate tasks.These changes provide a solid foundation for maintaining high code quality and set the stage for future enhancements, such as parallelizing clang-tidy checks. The consistent structure and naming conventions throughout the file contribute to its maintainability.
Great job on this refactoring! It significantly improves the project's linting capabilities while maintaining clarity and efficiency.
Taskfile.yml (3)
51-51: Excellent refactoring of the CMake configuration process.The introduction of the
config-cmake-projecttask and its usage here improves the modularity and readability of the build process. This change aligns well with best practices for task organization in build systems.
78-78: Excellent optimization for theemsdktask.Adding
run: "once"to theemsdktask is a smart optimization. This ensures that the Emscripten SDK is only installed once during the task lifecycle, preventing redundant installations and potentially saving significant time in subsequent builds.
Line range hint
1-138: Overall, excellent improvements to the build system structure and efficiency.The changes in this file significantly enhance the organization and efficiency of the build process:
- The new
config-cmake-projecttask improves modularity by separating CMake configuration.- The modification to the
clp-ffi-jstask to use the new configuration task streamlines the build process.- The addition of
run: "once"to theemsdktask optimizes performance by preventing redundant installations.These changes align perfectly with the PR objectives of improving the structure of the build system and optimizing execution flow. The code is well-organized, and the tasks are logically structured. Great work on enhancing the build system!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
README.md (2)
68-75: LGTM! Comprehensive and well-organized table of linting tasks.The table provides a clear and comprehensive list of linting tasks for both C++ and YAML files. The tasks are logically grouped and follow a consistent naming pattern, with clear distinctions between check and fix tasks.
For consistency, consider adding a blank line between the C++ tasks and the YAML tasks to improve readability:
| `lint:cpp-static-check` | Runs the C++ static analyzers. | | `lint:cpp-static-fix` | Runs the C++ static analyzers and fixes some violations. | + | `lint:yml-check` | Runs the YAML linters. | | `lint:yml-fix` | Runs the YAML linters and fixes some violations. |
61-76: Excellent addition to the README!This new section on "Running specific linters" is a valuable addition to the README. It aligns perfectly with the PR objectives of introducing tasks for C++ static analysis and provides clear, concise instructions for developers. The table format makes it easy for users to find the specific linting task they need, which will likely improve the development workflow and code quality.
As the project grows, consider organizing the README into separate files for different topics (e.g., a CONTRIBUTING.md file for development-related instructions) to keep the main README concise while still providing detailed information for contributors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- README.md (1 hunks)
🔇 Additional comments (2)
README.md (2)
61-64: LGTM! Clear and informative introduction.The introductory paragraph effectively explains the purpose of the new section and provides a good rationale for using specific linters. It's concise and consistent with the rest of the README.
66-67: LGTM! Well-structured table header.The table header is clear and well-formatted, providing appropriate column names for the content that follows.
| # When we eventually determine which errors can be safely fixed, we'll allow clang-tidy to | ||
| # fix them. | ||
| aliases: ["cpp-static-fix"] | ||
| sources: |
There was a problem hiding this comment.
Do we need to add G_EMSDK_CHECKSUM here?
There was a problem hiding this comment.
Yeah, my mistake. Let me restructure things to be clearer.
There was a problem hiding this comment.
Since we plan to move clang-format and clang-tidy into yscope-dev-utils, what do you think about the following? Move clang-format and clang-tidy's dependencies into cpp-format-* and cpp-static-* and make clang-format and clang-tidy idempotent.
| - "{{.G_SRC_CLP_FFI_JS_DIR}}/**/*.cpp" | ||
| - "{{.G_SRC_CLP_FFI_JS_DIR}}/**/*.h" | ||
| - "{{.G_SRC_CLP_FFI_JS_DIR}}/**/*.hpp" |
There was a problem hiding this comment.
Do we need to include all source files? Say if some header in clp-ffi-js' dependency changes, it would also impact the compilation in clp-ffi-js' source code, right?
There was a problem hiding this comment.
It would, but this task doesn't require compiling clp-ffi-js, right? (I might be misunderstanding your question.)
There was a problem hiding this comment.
I can't come up with an exact example since I'm not too familiar with clang-tidy rules. However, from my experience working with static analysis, any dependency changes can cause the checks in our code to fail. (In fact, sometimes it's easier to fix violations in the dependencies when there are too many violations due to a same issue in the dependency. )
Please correct me if this imaginary example is impossible - say we recently updated some function signature in CLP core code that some parameter changes its type from int64 to int32, but our clp-ffi-js code remains the same. If we keep passing an int64 value to the modified function, wouldn't a precision / value loss violation be raised?
If we don't include the header, which is used in static checks in the sources list here, the cpp-static-check would not run at all if I'm not wrong.
There was a problem hiding this comment.
Oh I see what you're saying. Yes, we should include the dependencies' source files.
There was a problem hiding this comment.
As instructed by the validation steps in the PR description, I made a missing header violation in StreamReader.hpp and running each lint task yield expected outputs.
The Taskfile changes meet our guidelines but it seems some extra sources can be added to the cpp-static-check sources list to improve accuracy of preventing unnecessary work.
Otherwise everything else lgtm and the PR title is fine to be used as the final commit message.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
lint-tasks.yml (3)
32-45: Well-structuredcpp-format-checktaskThe new
cpp-format-checktask is well-defined with appropriate source files and dependencies. The use of a YAML anchor for source files promotes code reuse, which is excellent.Consider adding a brief comment explaining the purpose of this task for improved clarity.
106-124: Well-configuredclang-tidytaskThe new
clang-tidytask is well-structured and includes necessary configuration for proper integration with the project's build environment. The inclusion of Emscripten's cflags is a good practice to ensure compatibility.Consider adding a brief comment explaining why Emscripten's cflags are necessary for this task, to improve clarity for future maintainers.
Line range hint
1-158: Overall improvements to lint tasks structureThe changes in this file significantly improve the organization and modularity of the linting tasks. The separation of C++ linting tasks into distinct categories has been well-implemented, aligning perfectly with the PR objectives. The file structure is now more maintainable and easier to understand.
To further enhance the file's documentation:
Consider adding a brief comment at the beginning of the file explaining the overall structure and purpose of these lint tasks. This would provide a quick overview for anyone new to the project or revisiting the file after some time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- Taskfile.yml (4 hunks)
- lint-tasks.yml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Taskfile.yml
🧰 Additional context used
🔇 Additional comments (5)
lint-tasks.yml (5)
4-5: Improved directory structure variablesThe introduction of
G_SRC_DIRand the modification ofG_SRC_CLP_FFI_JS_DIRto use it enhances the organization and flexibility of the directory structure. This change makes it easier to maintain and adjust paths in the future if needed.
23-31: Improved structure for C++ linting tasksThe modifications to
cpp-checkandcpp-fixtasks, along with the introduction of new subtasks for formatting and static analysis, align well with the PR objective of separating C++ linting tasks into distinct categories. This structure provides more granular control over different aspects of code quality checks, which is beneficial for managing the potentially slow clang-tidy process.
46-53: Consistentcpp-format-fixtaskThe
cpp-format-fixtask is well-defined and consistent withcpp-format-check. The reuse of source files through the YAML alias is an excellent practice, promoting code maintainability.
54-78: Comprehensivecpp-static-checktaskThe
cpp-static-checktask is well-structured and includes a comprehensive list of source files, addressing the concern raised in a past review comment about including dependency sources. The comment explaining the current limitation on automatic fixes is helpful and aligns with the PR objectives.Good job on incorporating feedback from the previous review and providing clear explanations in the comments.
98-104: Improvedclang-formattask structureThe modification of the
clang-formattask to usecmdinstead ofcmdsimproves readability and aligns with the suggestion from a past review comment. The functionality remains unchanged, which is good.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Taskfile.yml (1)
129-145: LGTM: Well-structured new CMake configuration taskThe new
config-cmake-projecttask is a well-structured addition that improves the project's build system:
- It encapsulates CMake configuration, enhancing modularity.
- The dependency on the
emsdktask ensures the correct environment.- Using the Emscripten toolchain file is appropriate for this project.
Suggestion for improvement: Consider adding a comment explaining the purpose of this task and why it's separated from the main build task. This would enhance maintainability for future developers.
Add a comment above the task definition, like this:
# Internal task to configure the CMake project using the Emscripten toolchain. # This is separated from the main build task to improve modularity and reusability. config-cmake-project: internal: true # ... rest of the task definition
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Taskfile.yml (4 hunks)
🧰 Additional context used
🔇 Additional comments (3)
Taskfile.yml (3)
39-39: LGTM: Improved task structure and dependenciesThe changes to the
clp-ffi-jstask enhance its structure and dependencies:
- Adding
CMakeCache.txtas a source file ensures the task reacts to changes in the CMake configuration.- Replacing direct CMake commands with the
config-cmake-projecttask improves modularity and maintainability.These modifications contribute to a more robust and flexible build process.
Also applies to: 52-52
Line range hint
1-145: LGTM: Consistent and well-structured TaskfileThe overall structure and consistency of the Taskfile are commendable:
- The new additions and modifications integrate seamlessly with existing tasks.
- Indentation and formatting are consistent throughout the file.
- Task dependencies and variable usage are appropriate and clear.
These characteristics contribute to a maintainable and easy-to-understand build configuration.
68-68: Verify impact ofrun: "once"directiveThe addition of
run: "once"to theemsdktask is a good optimization to prevent unnecessary re-execution. This can improve overall build performance.However, please verify that this doesn't prevent necessary updates to the emsdk environment in scenarios where it might need to be refreshed or updated.
To ensure this change doesn't introduce any issues, please run the following test:
kirkrodrigues
changed the title
kirkrodrigues
changed the title
|
How's the title now? |
Looks good |
Description
This PR adds tasks to run clang-tidy on our C++ code. Since clang-tidy is quite slow, we separate the C++ linting tasks into formatters and static analyzers. The updates to the README describe the new tasks.
We plan to parallelize clang-tidy's checking but it's more work than can be done in this scope of this PR. Similarly (also noted in the code), we currently don't support automated clang-tidy fixes since they can be inaccurate.
Validation performed
StreamReader::m_ts_patterntoStreamReader::mTsPatternand then:task lint:checkfailed due to the naming violation.task lint:cpp-checkfailed due to the naming violation.task lint:cpp-static-checkfailed due to the naming violation.task lint:fixfailed due to the naming violation.task lint:cpp-fixfailed due to the naming violation.task lint:cpp-static-fixfailed due to the naming violation.task lint:checkfailed due to the violation.task lint:cpp-checkfailed due to the violation.task lint:cpp-format-checkfailed due to the violation.task lint:fixremoved the extra whitespace.task lint:cpp-fixremoved the extra whitespace.task lint:cpp-format-fixremoved the extra whitespace.task lint:cpp-checktwice to ensure no checks were re-run, changed one of the submodules, and thentask lint:cpp-checkdetected the changes and re-rancpp-static-check.Summary by CodeRabbit
cpp-format-check,cpp-static-check, andclang-tidyin the linting configuration.clang-tidyand maintained existing versions for other linting tools.config-cmake-projecttask.