Repository navigation
feat: run clang-tidy - #200
Conversation
43d2698 to
3661e1a
Compare
7f2554c to
60e0239
Compare
60e0239 to
806591a
Compare
Add clang-tidy support in CompilationParams Remove the legacy fast check filter option
…dy header - Removed the previous configuration for clang-tidy header generation in CMake. - Updated include directories in both CMake and xmake to reference the new generated header location. - Added a new generated header file for clang-tidy configuration with private restrictions.
0449468 to
3836488
Compare
…nd action cleanup
|
There are some todos:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
include/Compiler/Diagnostic.h (2)
7-8: Prefer forward declaration over include.The include of
Compiler/Tidy.his only needed for the pointer membertidy::ClangTidyChecker*at line 61. A forward declaration would reduce compilation dependencies and improve build times.Apply this diff:
-#include "Compiler/Tidy.h" +namespace clice::tidy { +class ClangTidyChecker; +}
59-62: Encapsulate the public data member.The public data member
checkerbreaks encapsulation and allows external code to modify it arbitrarily. Consider making it private with a setter/getter for better control and maintainability. Additionally, the class and member lack documentation explaining their purpose and lifecycle.Apply this diff:
class DiagnosticCollector : public clang::DiagnosticConsumer { public: - tidy::ClangTidyChecker* checker = nullptr; + /// Set the clang-tidy checker for diagnostic processing. + void setChecker(tidy::ClangTidyChecker* checker) { this->checker = checker; } + + /// Get the clang-tidy checker. + tidy::ClangTidyChecker* getChecker() const { return checker; } + +private: + tidy::ClangTidyChecker* checker = nullptr; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
include/Compiler/Diagnostic.h(3 hunks)
⏰ 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). (8)
- GitHub Check: build (windows-2025)
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: macos (macos-15, release)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: windows (windows-2025)
- GitHub Check: linux (ubuntu-24.04, debug)
🔇 Additional comments (1)
include/Compiler/Diagnostic.h (1)
78-78: LGTM!The return type change to
DiagnosticCollector*is appropriate for the clang-tidy integration, allowing callers to access and configure the checker member.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Compiler/Compilation.cpp (1)
256-265: Preamble still not excluded from clang-tidy traversal.The comment states "AST traversals should exclude the preamble, to avoid performance cliffs," but the current filtering does not exclude preamble declarations. The
is_clangd_top_level_declfunction callsis_inside_main_file, which returnstruefor bothsm.getMainFileID()andsm.getPreambleFileID()(line 23 in src/AST/Utility.cpp). This means preamble declarations remain in the traversal scope, potentially causing the performance issues mentioned.To exclude preamble declarations, explicitly filter them out:
if(checker) { + auto& sm = instance->getSourceManager(); auto clangd_top_level_decls = top_level_decls; - std::erase_if(clangd_top_level_decls, - [](auto decl) { return !ast::is_clangd_top_level_decl(decl); }); + std::erase_if(clangd_top_level_decls, [&](clang::Decl* decl) { + if(!ast::is_clangd_top_level_decl(decl)) return true; + auto loc = sm.getExpansionLoc(decl->getLocation()); + auto fid = sm.getFileID(loc); + return fid == sm.getPreambleFileID(); // exclude preamble + }); // AST traversals should exclude the preamble, to avoid performance cliffs. // TODO: is it okay to affect the unit-level traversal scope here? instance->getASTContext().setTraversalScope(clangd_top_level_decls); checker->finder.matchAST(instance->getASTContext()); }src/Compiler/Tidy.cpp (1)
314-316: Race condition: static allocator and saver are not thread-safe.The static
allocatorandsaverare shared across all threads. If the server runs compilations in parallel (which is typical), multiple threads will access and modify these static variables concurrently, leading to race conditions and potential memory corruption.These should be instance members of
ClangTidyCheckerto ensure thread safety:
- Add members to
ClangTidyCheckerinsrc/Compiler/TidyImpl.h:llvm::BumpPtrAllocator allocator; llvm::StringSaver saver;
- Initialize
saverin the constructor:ClangTidyChecker::ClangTidyChecker(std::unique_ptr<ClangTidyOptionsProvider> provider) : context(std::move(provider)), saver(allocator) {}
- Use the instance members here:
- static llvm::BumpPtrAllocator allocator; - static llvm::StringSaver saver(allocator); - diag.id.name = saver.save(tidy_diag); + diag.id.name = saver.save(tidy_diag);
🧹 Nitpick comments (4)
src/Compiler/Tidy.cpp (4)
143-149: Add braces for single-statement conditionals.The codebase prefers braces even for single-statement blocks. This improves consistency and reduces the chance of errors when code is later modified.
Based on coding guidelines.
Apply this diff:
- if(!opts.Checks || opts.Checks->empty()) { + if(!opts.Checks || opts.Checks->empty()) { opts.Checks = default_checks; } // clang::clangd::disableUnusableChecks - if(opts.Checks && !opts.Checks->empty()) { + if(opts.Checks && !opts.Checks->empty()) { opts.Checks->append(bad_checks); }
170-212: Consider adding braces for single-statement conditionals.Multiple single-statement
ifblocks throughout this constructor lack braces (lines 178, 179, 195, 199, 203). While the code is functional, the codebase prefers braces for all conditionals to maintain consistency.Based on coding guidelines.
220-265: Consider adding braces for single-statement conditionals.Several single-statement blocks lack braces (lines 227, 233, 242, 246, 250). For consistency with codebase conventions, consider adding braces.
Based on coding guidelines.
370-375: Consider adding braces for single-statement conditionals.The conditional blocks at lines 370-371 and 373-374 lack braces. For consistency with codebase conventions, consider adding them.
Based on coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
include/AST/Utility.h(1 hunks)src/AST/Utility.cpp(2 hunks)src/Compiler/Compilation.cpp(5 hunks)src/Compiler/Tidy.cpp(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
include/AST/Utility.h (2)
src/AST/Utility.cpp (4)
is_inside_main_file(19-24)is_inside_main_file(19-19)is_clangd_top_level_decl(87-99)is_clangd_top_level_decl(87-87)src/Compiler/Compilation.cpp (2)
decl(28-39)decl(28-28)
src/AST/Utility.cpp (1)
src/Compiler/Compilation.cpp (2)
decl(28-39)decl(28-28)
src/Compiler/Compilation.cpp (3)
src/Compiler/Diagnostic.cpp (2)
create(260-262)create(260-260)src/Compiler/Tidy.cpp (2)
configure(334-405)configure(334-335)src/AST/Utility.cpp (2)
is_clangd_top_level_decl(87-99)is_clangd_top_level_decl(87-87)
src/Compiler/Tidy.cpp (3)
src/Compiler/TidyImpl.h (1)
ClangTidyChecker(16-31)src/Compiler/Diagnostic.cpp (2)
level(213-250)level(213-214)src/AST/Utility.cpp (2)
is_inside_main_file(19-24)is_inside_main_file(19-19)
⏰ 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). (8)
- GitHub Check: build (windows-2025)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-15)
- GitHub Check: macos (macos-15, release)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: windows (windows-2025)
🔇 Additional comments (10)
include/AST/Utility.h (1)
18-22: LGTM!The new function declarations are well-documented and provide clear utility for identifying main-file locations and top-level declarations. These support the clang-tidy integration workflow.
src/AST/Utility.cpp (3)
19-24: LGTM!The implementation correctly checks if a location is in the main file or preamble. The inclusion of preamble is intentional based on the function's purpose.
68-85: LGTM!The template specialization kind helpers are correctly implemented and provide clean abstractions for checking whether a declaration is an implicit template instantiation.
87-99: LGTM!The implementation correctly identifies top-level declarations in the main file, appropriately filtering out implicit template instantiations and ObjCMethodDecl. This aligns with clangd's behavior for clang-tidy integration.
src/Compiler/Compilation.cpp (5)
1-9: LGTM!The new includes properly support the clang-tidy integration by bringing in the necessary tidy implementation headers and AST utilities.
166-170: LGTM!The diagnostic collector is correctly instantiated and passed to the diagnostics engine, enabling the clang-tidy integration to hook into the diagnostic flow.
210-218: LGTM!The clang-tidy checker is correctly instantiated and configured when enabled, with proper lifecycle management through the diagnostic_collector.
267-270: LGTM with context.This workaround addresses the EOF diagnostic flush issue where clang-tidy checks flush diagnostics at EOF but calling
Action->EndSourceFile()would destroy the ASTContext. Callingpp.EndSourceFile()is a reasonable compromise, though as noted in the comment, it's not ideal. This approach is borrowed from clangd's handling of the same issue.
280-283: LGTM!Nullifying the checker pointer in the diagnostic_collector prevents dangling pointer issues after the checker's unique_ptr goes out of scope. Good defensive programming.
src/Compiler/Tidy.cpp (1)
73-81: LGTM!The function correctly filters fast checks from the provided factories, creating a new registry with only the checks marked as fast.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Compiler/Compilation.cpp (1)
265-274: Preamble filtering still missing; this remains unaddressed.The previous review correctly identified that preamble declarations are not excluded before
setTraversalScopeandmatchAST. The current code filters withis_clangd_top_level_decl()but still includes preamble declarations that were collected at lines 37-39. This can cause performance issues as noted in the comment on line 270.
🧹 Nitpick comments (1)
src/Compiler/Compilation.cpp (1)
34-39: Update comment to reflect actual behavior.The comment states "Check whether the location is inside the main file" but the code accepts both preamble and main file declarations. This discrepancy can confuse future readers.
Apply this diff:
- /// Check whether the location is inside the main file. + /// Check whether the location is inside the main file or preamble. location = src_mgr.getExpansionLoc(location); auto fid = src_mgr.getFileID(location); if(!(fid == src_mgr.getPreambleFileID() || fid == src_mgr.getMainFileID())) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/AST/Utility.h(1 hunks)src/AST/Utility.cpp(1 hunks)src/Compiler/Compilation.cpp(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- include/AST/Utility.h
🧰 Additional context used
🧬 Code graph analysis (2)
src/Compiler/Compilation.cpp (3)
src/AST/Utility.cpp (2)
is_implicit_template_instantiation(76-78)is_implicit_template_instantiation(76-76)src/Compiler/Diagnostic.cpp (2)
create(260-262)create(260-260)src/Compiler/Tidy.cpp (2)
configure(334-405)configure(334-335)
src/AST/Utility.cpp (1)
src/Compiler/Compilation.cpp (2)
decl(28-48)decl(28-28)
🔇 Additional comments (8)
src/AST/Utility.cpp (1)
61-78: LGTM! Clean utility implementation for template instantiation checking.The layered design is well-structured: the generic template handles type-specific checks, the inline overload covers the three standard declaration types that support template specialization, and the public wrapper provides a clear API for the common use case of detecting implicit instantiations. The implementation correctly handles edge cases (nullptr via
dyn_cast) and follows the existing predicate patterns in the file.src/Compiler/Compilation.cpp (7)
41-45: LGTM! Implicit template instantiation filtering added.The addition of
is_implicit_template_instantiationfiltering incollect_declaligns with the previous review feedback and correctly excludes these declarations from collection.
175-179: LGTM! Diagnostic collector pattern enables clang-tidy integration.The introduction of
diagnostic_collectoris the correct approach for enabling clang-tidy to access and emit diagnostics through the compilation pipeline.
211-211: LGTM! Top-level declarations now collected for clang-tidy.Extending the collection condition to include
params.clang_tidyis necessary for clang-tidy to traverse the AST.
219-227: LGTM! Clang-tidy checker setup is correctly positioned.The checker is configured after
BeginSourceFile(which may create a new preprocessor) and beforeExecute, which is the correct lifecycle position. Storing the checker pointer indiagnostic_collectorenables proper integration with the diagnostics engine.
289-292: LGTM! Checker pointer cleanup prevents dangling reference.Nullifying
diagnostic_collector->checkerbefore thecheckerunique_ptr goes out of scope correctly prevents dangling pointer access.
297-297: No issues identified with the interested FileID source change.The switch to
instance->getSourceManager().getMainFileID()is correct and consistent:
- The
interestedfield is used only in two accessor methods that retrieve it directly- Both
interestedandsrc_mgrare now sourced frominstance->getSourceManager(), maintaining internal consistency- The documentation explicitly states interested should equal the main file ID of the source file
- This consolidates to the standard pattern used throughout the codebase
276-279: EOF handling approach is correct and properly manages ASTContext lifetime.The workaround is intentional and sound. The action object remains alive after
pp.EndSourceFile()(line 279) and is later moved into theCompilationUnit::Implstruct (line 303). The ASTContext continues to be used by subsequent code (e.g., line 285–286 access Sema), confirming it's not destroyed prematurely. Early error/cancellation paths call the fullaction->EndSourceFile()only when clang-tidy is skipped, avoiding the unnecessary ASTContext preservation. No resource leaks or lifecycle issues exist.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Compiler/Compilation.cpp (2)
37-39: Preamble declarations should be excluded when collecting for clang-tidy.The current logic includes declarations from both the preamble and main file. For clang-tidy traversal (enabled via
params.clang_tidyat line 211), preamble declarations should be excluded to avoid significant performance degradation.This issue was previously flagged but remains unresolved.
Apply this diff to exclude preamble declarations:
- if(!(fid == src_mgr.getPreambleFileID() || fid == src_mgr.getMainFileID())) { - return; - } + if(fid != src_mgr.getMainFileID()) { + return; + }
265-271: Traversal scope includes preamble declarations despite comment.The comment indicates preamble should be excluded, but
top_level_declsincludes preamble declarations (collected at lines 37-39). This can cause significant performance degradation during clang-tidy AST traversal.Fixing the collection logic at lines 37-39 will resolve this issue. Alternatively, filter here before setting the traversal scope as suggested in the previous review.
🧹 Nitpick comments (1)
src/Compiler/Compilation.cpp (1)
294-294: Minor consistency improvement.Using
instance->getSourceManager()instead ofpp.getSourceManager()is more consistent with the instance-scoped lifecycle management and aligns with other accesses in the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Compiler/Compilation.cpp(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Compiler/Compilation.cpp (3)
src/AST/Utility.cpp (2)
is_implicit_template_instantiation(76-78)is_implicit_template_instantiation(76-76)src/Compiler/Diagnostic.cpp (2)
create(260-262)create(260-260)src/Compiler/Tidy.cpp (2)
configure(334-405)configure(334-335)
⏰ 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). (8)
- GitHub Check: build (windows-2025)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: macos (macos-15, release)
- GitHub Check: windows (windows-2025)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: build (macos-15)
🔇 Additional comments (6)
src/Compiler/Compilation.cpp (6)
41-45: Correctly filters implicit template instantiations.This filtering aligns with clangd's approach and addresses the previous discussion about excluding implicit template instantiations from clang-tidy traversal.
175-179: Diagnostic collector integration looks correct.The use of the factory method and passing the collector to
createDiagnosticsproperly integrates the new diagnostic collection architecture for clang-tidy.
211-211: Correctly enables top-level declaration collection for clang-tidy.This change ensures that top-level declarations are collected when clang-tidy is enabled, which is necessary for the AST matcher to function.
219-227: Clang-tidy checker setup is correct.The checker configuration, lifetime management, and integration with the diagnostic collector are properly implemented.
273-276: EndSourceFile workaround is acceptable but needs future refinement.The approach of calling only
pp.EndSourceFile()instead ofaction->EndSourceFile()is a known workaround to preserve the ASTContext for clang-tidy diagnostic flushing at EOF. The PR objectives acknowledge this as a TODO that requires a more robust solution.Consider tracking this as a technical debt item for future refinement.
286-289: Proper cleanup of checker pointer.Nullifying the checker pointer in the diagnostic collector prevents dangling references and is good defensive programming practice.
3eb542b to
f722576
Compare
f722576 to
993f3d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Compiler/Compilation.cpp (1)
257-263: Preamble declarations still not excluded (unresolved from past review).The comment states "AST traversals should exclude the preamble, to avoid performance cliffs," but
top_level_declsstill includes preamble declarations. Looking atast::is_inside_main_filein src/AST/Utility.cpp (lines 18-23), it returnstruefor bothgetMainFileID()andgetPreambleFileID(), so preamble decls are collected and included in the traversal scope. This can cause significant performance degradation during clang-tidy analysis.Apply this diff to exclude preamble declarations before setting the traversal scope:
if(checker) { + auto& sm = instance->getSourceManager(); + auto preamble_fid = sm.getPreambleFileID(); + std::vector<clang::Decl*> filtered_decls; + for(auto* decl : top_level_decls) { + auto loc = sm.getExpansionLoc(decl->getLocation()); + if(sm.getFileID(loc) != preamble_fid) { + filtered_decls.push_back(decl); + } + } // AST traversals should exclude the preamble, to avoid performance cliffs. // TODO: is it okay to affect the unit-level traversal scope here? - instance->getASTContext().setTraversalScope(top_level_decls); + instance->getASTContext().setTraversalScope(filtered_decls); checker->finder.matchAST(instance->getASTContext()); }
🧹 Nitpick comments (1)
include/AST/Utility.h (1)
18-19: Consider documenting preamble file behavior.The implementation also returns
truefor locations in the preamble file (viasm.getPreambleFileID()). The comment could mention this for clarity, e.g., "Checks whether the location is inside the main file or preamble."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/AST/Utility.h(1 hunks)src/AST/Utility.cpp(2 hunks)src/Compiler/Compilation.cpp(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/AST/Utility.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
include/AST/Utility.h (2)
src/AST/Utility.cpp (4)
is_inside_main_file(19-24)is_inside_main_file(19-19)is_implicit_template_instantiation(83-85)is_implicit_template_instantiation(83-83)src/Compiler/Compilation.cpp (2)
decl(28-40)decl(28-28)
src/Compiler/Compilation.cpp (3)
src/AST/Utility.cpp (4)
is_inside_main_file(19-24)is_inside_main_file(19-19)is_implicit_template_instantiation(83-85)is_implicit_template_instantiation(83-83)src/Compiler/Diagnostic.cpp (2)
create(260-262)create(260-260)src/Compiler/Tidy.cpp (2)
configure(334-405)configure(334-335)
⏰ 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). (7)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: windows (windows-2025)
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
🔇 Additional comments (8)
include/AST/Utility.h (1)
21-22: LGTM!The declaration is correct and integrates well with the codebase. The function is properly used in
Compilation.cppto filter implicit template instantiations during decl collection.src/Compiler/Compilation.cpp (7)
1-4: LGTM: Necessary includes for clang-tidy integration.The added includes support the clang-tidy integration and utility functions used throughout the file.
Also applies to: 9-9
28-40: LGTM: Correctly filters implicit template instantiations.The implementation properly excludes implicit template instantiations from the top-level declarations, aligning with the resolution in past review comments. The cast to
NamedDeclbefore checking is appropriate.
167-171: LGTM: Diagnostic collector setup for tidy integration.The diagnostic flow now uses a collector that can hold a reference to the clang-tidy checker, enabling proper diagnostic handling during tidy analysis.
203-203: LGTM: Enables declaration collection for clang-tidy.Collecting top-level declarations when
clang_tidyis enabled is necessary for AST matcher traversal performed later.
265-268: LGTM: EOF workaround aligns with PR objectives.The
pp.EndSourceFile()call addresses the clangd-origin TODO mentioned in the PR objectives: it allows clang-tidy checks to flush diagnostics at EOF while keeping the ASTContext alive (which would be destroyed byAction->EndSourceFile()). This is an acknowledged workaround as indicated by the "XXX: This is messy" comment.
278-281: LGTM: Proper cleanup to avoid dangling pointer.Nullifying the checker pointer in the diagnostic collector before the checker is destroyed prevents potential use-after-free issues.
286-286: LGTM: Consistent FileID source.Using
instance->getSourceManager()instead ofpp.getSourceManager()is more direct and consistent with other usage in the file.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Compiler/Compilation.cpp(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Compiler/Compilation.cpp (3)
src/AST/Utility.cpp (4)
is_inside_main_file(19-24)is_inside_main_file(19-19)is_implicit_template_instantiation(83-85)is_implicit_template_instantiation(83-83)src/Compiler/Diagnostic.cpp (2)
create(260-262)create(260-260)src/Compiler/Tidy.cpp (2)
configure(334-405)configure(334-335)
⏰ 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). (8)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
- GitHub Check: windows (windows-2025)
- GitHub Check: build (macos-15)
- GitHub Check: macos (macos-15, release)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: linux (ubuntu-24.04, debug)
Run clang-tidy on
build_ast. This PR only focuses on integrating clang-tidy into clice, and might have following issues:Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Build
Tests