Add tasks to perform C++ linting with clang-format v18. #17
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![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 (2)
lint-tasks.yml (2)
21-33: LGTM: C++ check task well-definedThe
cpp-checktask is well-structured and comprehensive. It correctly defines the source files to be checked and uses theclang-formattask with the appropriate--dry-runflag. The use of an anchor for the source files list is a good practice for maintainability.One minor suggestion:
Consider adding a comment explaining the purpose of including the
.clang-formatfiles in the sources list. This might not be immediately obvious to all developers.
56-69: LGTM: clang-format task well-implementedThe
clang-formattask is well-structured and flexible. It correctly uses a virtual environment, handles file types appropriately, and uses safe practices for dealing with filenames. The use of theFLAGSvariable allows for reuse in both check and fix scenarios.One suggestion for improvement:
Consider adding a
set -euo pipefailat the beginning of the script to ensure it fails fast on any error. This can help catch issues early in the pipeline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- lint-requirements.txt (1 hunks)
- lint-tasks.yml (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- lint-requirements.txt
🔇 Additional comments (6)
lint-tasks.yml (6)
4-4: LGTM: New variable for C++ source directoryThe addition of
G_SRC_CLP_FFI_JS_DIRis appropriate and follows the existing naming convention. This variable will be useful for specifying the source directory in C++ linting tasks.
10-11: LGTM: C++ check task added to main check commandThe addition of the
cpp-checktask to the maincheckcommand is appropriate and aligns with the PR objective of introducing C++ linting. The task order and naming convention are consistent with the existing structure.
15-16: LGTM: C++ fix task added to main fix commandThe addition of the
cpp-fixtask to the mainfixcommand is appropriate and complements thecpp-checktask. The task order and naming convention are consistent with the existing structure and the changes made to thechecktask.
34-40: LGTM: C++ fix task well-definedThe
cpp-fixtask is well-structured and consistent with thecpp-checktask. It efficiently reuses the source files definition and correctly uses theclang-formattask with the-iflag for in-place editing. This implementation aligns well with the fix functionality.
Line range hint
41-55: LGTM: YAML task reorganizationThe repositioning of the
ymltask definition improves the overall organization of the file by grouping similar tasks together. The functionality remains unchanged, which is appropriate given the PR's focus on adding C++ linting.
Line range hint
1-69: Overall assessment: Well-implemented C++ linting integrationThe changes in this file successfully implement C++ linting tasks using clang-format, as per the PR objectives. The new tasks are well-structured, consistent with existing patterns, and integrate smoothly with the existing YAML linting tasks. The use of variables and anchors promotes maintainability, and the task definitions are clear and logical.
A few minor suggestions were made for additional comments and error handling, but overall, this is a solid implementation that achieves the stated goals of the PR.
| sources: &cpp_source_files | ||
| - "{{.G_SRC_CLP_FFI_JS_DIR}}/.clang-format" | ||
| - "{{.G_SRC_CLP_FFI_JS_DIR}}/**/*.cpp" | ||
| - "{{.G_SRC_CLP_FFI_JS_DIR}}/**/*.h" |
There was a problem hiding this comment.
Just curious since I'm not too familiar with our C++ code conventions, if we're not sticking with *.hpp for headers, when do we use *.h?
There was a problem hiding this comment.
.hpp are meant to be C++ headers. .h are meant to be C-compatible headers.
There was a problem hiding this comment.
Right, are there cases we need our own C-compatible headers in src/clp-ffi-js?
There was a problem hiding this comment.
Not that I can think of, but this is more of a default setting in case we have them one day. I could see us forgetting about it and then the file doesn't get formatted properly, lol.
| @@ -30,6 +53,20 @@ tasks: | |||
| lint-tasks.yml \ | |||
| Taskfile.yml | |||
|
|
|||
| clang-format: | |||
There was a problem hiding this comment.
Just curious, is there a future plan to have this task moved into yscope-dev-utils and also have the venv setup task done from there?
There was a problem hiding this comment.
I hadn't considered it, but yeah, it probably makes sense, especially now that we can specify maps/arrays as arguments to a task.
There was a problem hiding this comment.
Following the Validation steps in the PR description, validated by making lint violations and observing the violation by the lint:check / lint:cpp-check task. e.g.,
/path/to/clp-ffi-js/src/clp_ffi_js/ir/StreamReaderDataContext.hpp:30:56: error: code should be clang-formatted [-Wclang-format-violations]
m_deserializer{std::move(deserializer)} { }
^
task: Failed to run task "lint:check": exit status 1
Running lint:fix or lint:cpp-fix fixed the violation inline.
The PR title is fine for the final message. If we're to upgrade clang-format in the near future, it might be good to mention the exact version in the title / message.
Co-authored-by: Junhao Liao <[email protected]>
kirkrodrigues
changed the title
Sure, updated the title. |
Description
This PR adds the
lint:cpp-checkandlint:cpp-fixtasks to perform C++ linting. Currently, the tasks only run clang-format but clang-tidy will be added in a subsequent PR.Validation performed
task lint:checksucceeded.task lint:fixsucceeded.task lint:cpp-checksucceeded.task lint:cpp-fixsucceeded.src/clp_ffi_js/ir/StreamReader.cppand then:task lint:checkdetected the violation.task lint:cpp-checkdetected the violation.task lint:fixfixed the violation.task lint:cpp-fixfixed the violation.Summary by CodeRabbit
New Features
clang-formatrequirement for improved C++ code formatting.Improvements