feat(lint): Add tasks for linting C++ files with clang-tidy; Refactor tasks for linting C++ files (resolves #64). #125
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis set of changes restructures the project's linting and formatting infrastructure. It deletes the Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Taskfile Lint
participant Clang-Format
participant Clang-Tidy
participant Yamllint
Developer->>Taskfile Lint: Run check or fix tasks
Taskfile Lint->>Clang-Format: Check/Format C++ (examples, log-surgeon)
Taskfile Lint->>Clang-Tidy: Static analysis (examples, log-surgeon)
Taskfile Lint->>Yamllint: Lint YAML/config files
Clang-Format-->>Taskfile Lint: Formatting results
Clang-Tidy-->>Taskfile Lint: Static analysis results
Yamllint-->>Taskfile Lint: YAML lint results
Taskfile Lint-->>Developer: Reports status and suggestions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@coderabbitai Please review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@davidlion I'll review this PR for you. Let me gather information about the changes first. 🏁 Script executed: Length of output: 452 🏁 Script executed: Length of output: 2070 🏁 Script executed: Length of output: 14211 @davidlion I've reviewed the changes in this PR. This is a significant refactoring of your linting and build infrastructure. Here's my comprehensive analysis: Summary of ChangesThis PR removes the local
📋 Detailed Review✅ Positive Changes
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
taskfiles/build.yaml (1)
114-127: Optional: exposegenerateunder a clearer name
generateis currently internal but referenced externally asbuild:generate. Renaming it to something likecmake-generate(and keeping it internal) would make intent unmistakable and avoid confusion with other potential “generate” helpers down the road.taskfiles/lint.yaml (4)
50-69:check-cpp-static-diffhard-codes adebugbuildUsing a single configuration is fine, but if contributors compile with
releaseonly, the compilation database (compile_commands.json) may be stale and clang-tidy findings inaccurate. Consider parameterising build type or documenting the requirement to keep the debug build up-to-date.
70-132: Huge exclude list risks hiding regressions
check-cpp-static-fullskips 40+ source files. While understandable during migration, it risks entrenching unchecked code.Propose adding a CI warning whenever the exclude list grows and encouraging PR authors to “touch” excluded files to clean them up incrementally.
133-149: Yamllint targets.clang-format, which may not be YAMLLinting
.clang-formatwith yamllint will flag non-YAML syntax. Unless the external config is actually YAML-based, you may want to drop that file from the list or switch toclang-format-style-optionsvalidation.
179-182: Ensuresymlink-cpp-lint-configs.shis idempotentRepeated runs of lint tasks should not fail if the symlink already exists. A quick
ln -sfin the script would guarantee that.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.clang-format(0 hunks)lint-requirements.txt(1 hunks)taskfiles/build.yaml(2 hunks)taskfiles/lint.yaml(2 hunks)
💤 Files with no reviewable changes (1)
- .clang-format
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (macos-15, debug)
🔇 Additional comments (5)
taskfiles/build.yaml (1)
83-86: 👍 Build task now cleanly depends ongenerateSwitching the
buildtask to depend on an internalgeneratestep separates configuration from compilation and improves reuse (lint tasks can now callbuild:generatedirectly). No concerns here.taskfiles/lint.yaml (4)
4-6: Good call including the build taskfileLinking the
buildTaskfile lets lint tasks bootstrap CMake without duplicating logic. Looks solid.
13-14: Verify thattools/yscope-dev-utilsis always present
G_UTILS_CONFIGS_DIRassumes the submodule (or vendored copy) is checked out. If a user clones with--depth 1and forgets submodules, every lint task will fail.Add a short pre-flight check (or documentation note) reminding contributors to fetch submodules.
31-49: Reference to.clang-formatmay be stale
check-cpp-formatlists{{.ROOT_DIR}}/.clang-formatinsources, but the PR deletes that file and relies on the symlink script. If the symlink fails (e.g., beforecpp-lint-configsruns) the taskfile checksum logic could break.Confirm that
symlink-cpp-lint-configs.shexecutes early enough and that the resulting link satisfies thesourceswatcher.
167-178:fix-cpp-formatcorrectly invokes in-place formattingNice touch using
-ihere whilecheck-cpp-formatstays read-only. This keeps the interface symmetrical with other fix/check pairs.
davidlion
changed the title
…ples files excluded.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/build.yaml(1 hunks).github/workflows/lint.yaml(1 hunks)README.md(3 hunks)examples/CMakeLists.txt(1 hunks)examples/buffer-parser.cpp(2 hunks)examples/common.cpp(2 hunks)examples/common.hpp(1 hunks)examples/reader-parser.cpp(2 hunks)src/log_surgeon/Token.cpp(1 hunks)taskfiles/lint.yaml(2 hunks)tools/init.sh(0 hunks)tools/yscope-dev-utils(1 hunks)
💤 Files with no reviewable changes (1)
- tools/init.sh
✅ Files skipped from review due to trivial changes (5)
- src/log_surgeon/Token.cpp
- tools/yscope-dev-utils
- README.md
- .github/workflows/build.yaml
- .github/workflows/lint.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
examples/common.hppexamples/common.cppexamples/buffer-parser.cppexamples/reader-parser.cpp
🧠 Learnings (3)
📚 Learning: in `src/log_surgeon/finite_automata/regexnfastate.hpp`, the constructor `regexnfastate(std::set
Learnt from: SharafMohamed
PR: y-scope/log-surgeon#48
File: src/log_surgeon/finite_automata/RegexNFAState.hpp:0-0
Timestamp: 2024-11-13T20:02:13.737Z
Learning: In `src/log_surgeon/finite_automata/RegexNFAState.hpp`, the constructor `RegexNFAState(std::set<Tag const*> tags, RegexNFAState const* dest_state)` has been updated to use `std::vector<Tag const*> tags` instead of `std::set`.
Applied to files:
examples/common.hppexamples/common.cppexamples/buffer-parser.cppexamples/reader-parser.cpp
Learnt from: SharafMohamed
PR: y-scope/log-surgeon#48
File: src/log_surgeon/finite_automata/RegexNFAState.hpp:0-0
Timestamp: 2024-11-13T20:02:13.737Z
Learning: In `src/log_surgeon/finite_automata/RegexNFAState.hpp`, the constructor `RegexNFAState(std::set<Tag const*> tags, RegexNFAState const* dest_state)` has been updated to use `std::vector<Tag const*> tags` instead of `std::set`.
examples/common.hppexamples/common.cppexamples/buffer-parser.cppexamples/reader-parser.cpp📚 Learning: in the `log_surgeon` project, header guards in c++ header files should include `_hpp` at the end to ...
Learnt from: SharafMohamed
PR: y-scope/log-surgeon#56
File: src/log_surgeon/finite_automata/RegisterHandler.hpp:0-0
Timestamp: 2024-11-27T21:56:13.425Z
Learning: In the `log_surgeon` project, header guards in C++ header files should include `_HPP` at the end to match the filename. For example, in `RegisterHandler.hpp`, the header guard should be `LOG_SURGEON_FINITE_AUTOMATA_REGISTER_HANDLER_HPP`.
Applied to files:
examples/common.hpp
📚 Learning: cmake's find_package() command automatically honors _root variables (e.g., log_surgeon_...
Learnt from: davidlion
PR: y-scope/log-surgeon#142
File: taskfiles/examples.yaml:63-76
Timestamp: 2025-07-22T02:00:47.620Z
Learning: CMake's find_package() command automatically honors <PackageName>_ROOT variables (e.g., LOG_SURGEON_ROOT) during its search process. The CMakeLists.txt file does not need to explicitly reference these _ROOT variables - they are built into CMake's package discovery mechanism.
Applied to files:
examples/CMakeLists.txt
🔇 Additional comments (15)
examples/CMakeLists.txt (1)
4-9: LGTM! Enables compile commands export for tooling support.This change correctly enables the generation of
compile_commands.json, which is essential for clang-tidy and other static analysis tools to function properly. The implementation follows CMake best practices with proper caching and descriptive documentation.taskfiles/lint.yaml (3)
3-12: LGTM! Improved modular structure with internal taskfile includes.The inclusion of separate taskfiles for
examplesandlog-surgeonwith internal scope improves modularity and follows the DRY principle. This structure will make maintenance easier and allows for better separation of concerns.
204-206: LGTM! Symlink configuration approach provides flexibility.The
cpp-lint-configsinternal task that creates symlinks to external configurations is a good approach. It allows for centralized configuration management while maintaining local accessibility.
17-17: Verify external dependency availability during linting tasks.The heavy reliance on
tools/yscope-dev-utils/exports/lint-configscreates a potential point of failure if the external repository is unavailable or incompatible.Consider adding a verification task to ensure external dependencies are available before running linting tasks:
#!/bin/bash # Verify that required external lint configurations are available if [[ ! -f "tools/yscope-dev-utils/exports/lint-configs/.clang-tidy" ]]; then echo "Error: External lint configurations not found" echo "Please ensure yscope-dev-utils submodule is properly initialized" exit 1 fiexamples/common.hpp (2)
4-4: LGTM! Addition of cstdint header for explicit type definitions.Adding the
<cstdint>header ensures explicit availability of standard integer types, which is a good practice for portability and clarity.
10-10: LGTM! Modernized function signature improves type safety.The change from
(int argc, char* argv[])to(std::vector<std::string> const& args)is an excellent modernization that:
- Improves type safety by using std::string instead of raw char pointers
- Uses const reference for efficiency and immutability
- Follows modern C++ best practices
examples/buffer-parser.cpp (2)
17-17: LGTM! Improved const correctness for schema_path parameter.Changing the parameter from
string&tostring const&prevents accidental modification of the input and follows const correctness best practices.
94-98: LGTM! Modernized argument handling with std::vector.The conversion from raw
argc/argvtostd::vector<std::string>is an excellent modernization that:
- Improves type safety by using std::string instead of char*
- Correctly excludes the program name using
argv + 1- Makes argument access cleaner and more readable
- Aligns with the updated
check_inputfunction signatureexamples/reader-parser.cpp (2)
16-16: LGTM! Improved const correctness for schema_path parameter.The change to
string const&prevents accidental modification and follows const correctness best practices, consistent with the modernization inbuffer-parser.cpp.
63-67: LGTM! Consistent modernization of argument handling.The conversion to
std::vector<std::string>maintains consistency with the changes inbuffer-parser.cppand provides the same benefits:
- Type safety with std::string
- Clean argument access via vector indexing
- Proper exclusion of program name
examples/common.cpp (5)
3-3: LGTM: Include addition is appropriate.The
<cstdint>include is correctly added to support theuint32_ttype used in theprint_timestamp_loglevelfunction parameter.
10-10: LGTM: Include addition aligns with function usage.The
<log_surgeon/Token.hpp>include is properly added to support theTokentype usage in theprint_timestamp_loglevelfunction.
15-31: Excellent refactoring: Function signature modernisation is well-implemented.The refactoring from traditional
int argc, char* argv[]tostd::vector<std::string> const&is executed correctly:
- Parameter validation properly uses
args.size()instead ofargc- File path access correctly uses
args[0]andargs[1]instead ofargv[1]andargv[2]- The logic maintains identical functionality while providing better type safety
- Follows the coding guideline by using
false == filesystem::exists(f)instead of!filesystem::exists(f)This change aligns with the coordinated updates mentioned in the AI summary across other example files.
19-29: LGTM: Output formatting consistency improvement.The change from
endlto\nis appropriate as it:
- Eliminates unnecessary buffer flushing for better performance
- Maintains consistent formatting throughout the function
- Aligns with modern C++ best practices for simple line breaks
49-49: LGTM: Consistent output formatting.The change from
endlto\nmaintains consistency with the formatting changes made throughout the file and improves performance by avoiding unnecessary buffer flushes.
davidlion
changed the title
davidlion
changed the title
| * [fmt] >= 8.0.1 | ||
| * [GSL] >= 4.0.0 | ||
| * [Task] >= 3.38 | ||
| * [uv] >= 0.7.10 |
There was a problem hiding this comment.
This doesn't match the version being installed. Is it suppose to be 0.8.4?
There was a problem hiding this comment.
It should reflect the minimum required version to work (uv in-particular is inherited from dev-utils). For CI we just use the latest stable so that we pick up any new changes. Ideally, we tested both sides (minimum and maximum versions), but it is awkward to setup.
| * [fmt] >= 8.0.1 | ||
| * [GSL] >= 4.0.0 | ||
| * [Task] >= 3.38 | ||
| * [uv] >= 0.7.10 |
There was a problem hiding this comment.
Does this need to reflect the llvm clang-tidy 20 install?
2 participants
Description
(This PR was split/refactored to depend on #142.)
Add tasks to run clang-tidy with an exclude list of all know files with issues (mimicking y-scope/clp#753). New PRs should attempt to resolve issues in their files and remove them from the exclude list.
A few other changes were needed to enable this:
.clang-formatis removed as we now link to the copy in yscope-dev-utils.tools/init.shscript as config files are setup through task.examples/CMakeLists.txtnow generatescompile_commands.jsonby default for clang-tidy to use.examples/common.*ppotherwise*-cpp-static-exampleswill fail to run as no files are input to clang-tidy (since they are all excluded).Updated and new public tasks (full list can be seen with
task -a):lint:check:check-cppcheck-yamllint:check-cpp:check-cpp-format-examplescheck-cpp-format-log-surgeoncheck-cpp-static-examplescheck-cpp-static-log-surgeonlint:check-cpp-format-examples: runs clang-format--dry-runonexamples/lint:check-cpp-format-log-surgeon: runs clang-format--dry-runonsrc/andtests/lint:check-cpp-static-examples(aliases:lint:fix-cpp-static-examples): runs clang-tidy onexamples/lint:check-cpp-static-log-surgeon(aliases:lint:fix-cpp-static-log-surgeon): runs clang-tidy onsrc/andtests/lint:check-yaml(aliases:lint:fix-yaml): runs yamllintlint:fix:fix-cppfix-yamllint:fix-cpp:fix-cpp-format-examplesfix-cpp-format-log-surgeonfix-cpp-static-examplesfix-cpp-static-log-surgeonlint:fix-cpp-format-examples: runs clang-format-ionexamples/lint:fix-cpp-format-log-surgeon: runs clang-format-ionsrc/andtests/Checklist
breaking change.
Validation performed
Tasks (and ci) passing.
Summary by CodeRabbit