Repository navigation
refactor: diagnostic handling - #337
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRefactors the compilation model to a ref-based CompilationUnitRef/Self with CompilationKind and CompilationStatus; public compile/preprocess/complete APIs now return CompilationUnit; diagnostics and clang-tidy are per-unit; many call sites and tests updated to use unit.completed()/unit.diagnostics() and CompilationKind. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as CompilationAPI
participant Unit as CompilationUnit
participant Self as CompilationUnitRef::Self
participant Diag as DiagnosticCollector
participant Tidy as ClangTidyChecker
Client->>API: compile(params)
activate API
API->>Unit: construct CompilationUnit(Self*)
activate Unit
Unit->>Self: initialize (remapped buffers, instance, checker)
activate Self
API->>Diag: create_diagnostic(Unit)
API->>Self: unit.configure_tidy() [optional]
Self->>Tidy: register checks (if configured)
API->>Self: run_clang()/frontend action
rect rgb(230,245,230)
Self->>Diag: emit diagnostics
Diag-->>Self: append to unit.diagnostics()
end
Self->>Unit: set status (Completed/Cancelled/SetupFail/FatalError)
deactivate Self
Unit-->>API: return CompilationUnit
API-->>Client: CompilationUnit
Client->>Unit: query (completed?/diagnostics()/status())
Unit-->>Client: results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (61)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
include/Compiler/Compilation.hinclude/Compiler/CompilationUnit.hsrc/Compiler/Compilation.cppsrc/Compiler/CompilationUnit.cppsrc/Compiler/CompilationUnitImpl.hsrc/Compiler/Module.cppsrc/Feature/CodeCompletion.cppsrc/Feature/SignatureHelp.cppsrc/Index/TUIndex.cppsrc/Index/USRGeneration.cppsrc/Server/Document.cppsrc/Server/Indexer.cpptests/unit/Compiler/DiagnosticTests.cpptests/unit/Compiler/ModuleTests.cpptests/unit/Compiler/PreambleTests.cpptests/unit/Compiler/TidyTests.cpptests/unit/Compiler/ToolchainTests.cpptests/unit/Test/Tester.cpp
🧰 Additional context used
🧬 Code graph analysis (10)
src/Server/Document.cpp (3)
src/Feature/DocumentLink.cpp (2)
document_links(11-40)document_links(11-11)src/Compiler/CompilationUnit.cpp (2)
diagnostics(227-229)diagnostics(227-227)src/Feature/Diagnostic.cpp (2)
diagnostics(9-121)diagnostics(9-9)
tests/unit/Compiler/DiagnosticTests.cpp (1)
src/Compiler/Compilation.cpp (6)
compile(357-362)compile(357-357)compile(364-391)compile(364-364)compile(393-413)compile(393-393)
src/Feature/SignatureHelp.cpp (1)
src/Compiler/Compilation.cpp (2)
complete(415-442)complete(415-415)
src/Index/TUIndex.cpp (1)
src/Index/MergedIndex.cpp (2)
relation(60-65)relation(60-60)
include/Compiler/CompilationUnit.h (1)
src/Compiler/CompilationUnit.cpp (4)
success(20-22)success(20-20)error(24-26)error(24-24)
src/Server/Indexer.cpp (1)
src/Index/TUIndex.cpp (2)
build(169-177)build(169-169)
src/Compiler/Compilation.cpp (2)
src/Compiler/CompilationUnit.cpp (6)
diagnostics(227-229)diagnostics(227-227)error(24-26)error(24-24)build_at(235-237)build_at(235-235)src/Compiler/Directive.cpp (2)
attach(265-268)attach(265-266)
include/Compiler/Compilation.h (2)
src/Compiler/Compilation.cpp (10)
preprocess(353-355)preprocess(353-353)compile(357-362)compile(357-357)compile(364-391)compile(364-364)compile(393-413)compile(393-393)complete(415-442)complete(415-415)tests/unit/Test/Tester.cpp (2)
compile(30-44)compile(30-30)
src/Feature/CodeCompletion.cpp (1)
src/Compiler/Compilation.cpp (2)
complete(415-442)complete(415-415)
tests/unit/Compiler/ModuleTests.cpp (2)
src/Compiler/Compilation.cpp (6)
compile(357-362)compile(357-357)compile(364-391)compile(364-364)compile(393-413)compile(393-393)tests/unit/Test/Tester.cpp (2)
compile(30-44)compile(30-30)
⏰ 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). (10)
- GitHub Check: xmake / build (macos-15, debug)
- GitHub Check: xmake / build (macos-15, releasedbg)
- GitHub Check: cmake / build (macos-15, Debug)
- GitHub Check: xmake / build (windows-2025, releasedbg)
- GitHub Check: xmake / build (ubuntu-24.04, releasedbg)
- GitHub Check: cmake / build (ubuntu-24.04, Debug)
- GitHub Check: xmake / build (ubuntu-24.04, debug)
- GitHub Check: clice / publish-clice (windows-2025, clice.zip, clice-x64-windows-msvc.zip, clice-symbol.zip, clice-x64-w...
- GitHub Check: cmake / build (windows-2025, RelWithDebInfo)
- GitHub Check: clice / publish-clice (macos-15, clice.tar.gz, clice-arm64-macos-darwin.tar.gz, clice-symbol.tar.gz, clic...
🔇 Additional comments (32)
src/Index/USRGeneration.cpp (1)
788-794: Good defensive fix to prevent null dereference.The additional null check prevents a crash when
hasTypeConstraint()returns true butgetTypeConstraint()returns null. The FIXME comment indicates this is a workaround for an underlying inconsistency in the AST API.Consider investigating why
hasTypeConstraint()andgetTypeConstraint()can be inconsistent—this should ideally be fixed at the source in the Clang AST implementation rather than guarded at each call site. No other unguarded uses ofgetTypeConstraint()were found in the file.include/Compiler/Compilation.h (1)
66-80: LGTM! Clean API refactor.The return type changes from
CompilationResulttoCompilationUnitare consistent across all five function declarations and align with the PR's objective to eliminatestd::expected-based error handling in favor ofsuccess()anderror()methods onCompilationUnit.src/Feature/SignatureHelp.cpp (1)
206-209: LGTM! Correct API usage.The code properly uses the new
unit.success()API to check compilation status. The FIXME comment appropriately indicates that actual error handling is deferred for future implementation.src/Server/Indexer.cpp (1)
14-14: LGTM! Proper API migration.The changes correctly adopt the new
CompilationUnitAPI:
- Error checking uses
unit.success()- Error messages accessed via
unit.error()- Unit passed directly to
TUIndex::build()instead of being dereferencedThe designated initializer syntax on line 14 is also a nice touch for clarity.
Also applies to: 28-33
src/Server/Document.cpp (2)
216-221: LGTM! Proper error handling in PCH build.The PCH compilation result is correctly checked using
unit.success(), error messages are extracted viaunit.error(), and the unit is passed directly todocument_links()without dereferencing.
330-352: LGTM! Consistent API usage in AST build.The AST compilation result handling correctly mirrors the PCH pattern:
- Status checked with
ast.success()- Errors logged using
ast.error()- AST passed directly to
diagnostics()and moved without dereferencingAll changes align with the new
CompilationUnitAPI.tests/unit/Compiler/TidyTests.cpp (1)
26-27: LGTM! Test correctly updated for new API.The test properly migrates from optional-like semantics (
has_value(),operator->) to the new direct value API (success(), direct member access). The test logic remains unchanged.tests/unit/Test/Tester.cpp (2)
34-34: LGTM! API migration correctly implemented.The changes correctly adapt to the new CompilationUnit API: explicit success check via
success()method and direct move semantics without dereferencing.Also applies to: 42-42
83-83: LGTM! Consistent API migration.The same API migration pattern is correctly applied in
compile_with_pch(), maintaining consistency with thecompile()method.Also applies to: 107-107, 115-115
include/Compiler/CompilationUnit.h (1)
55-58: LGTM! Clean and explicit API design.The new
success()anderror()accessors provide a clear, explicit interface for checking compilation status. Returningstd::stringby value forerror()is a reasonable trade-off—it avoids lifetime issues at the cost of a copy on error paths, which are not performance-critical.tests/unit/Compiler/ModuleTests.cpp (1)
30-30: LGTM! Test helper correctly updated.The explicit
success()check aligns with the new API and makes the test setup logic clear.src/Feature/CodeCompletion.cpp (1)
435-436: LGTM! Appropriate error handling for code completion.The variable rename from
infotounitbetter reflects the actual type. The explicitsuccess()check aligns with the new API. The TODO for error handling is appropriate—returning an empty completion list on failure provides graceful degradation for the user.tests/unit/Compiler/ToolchainTests.cpp (2)
76-77: LGTM! API migration is correct.The test correctly migrates to the new
CompilationUnitAPI usingsuccess()for status checks and direct member access for diagnostics.
115-116: LGTM! Consistent API migration.The Clang test case correctly adopts the same API pattern as the GCC test.
src/Compiler/Module.cpp (2)
109-111: LGTM! Correct error handling with new API.The error path correctly checks
unit.success()and propagates the error message viaunit.error()usingstd::unexpected.
113-118: LGTM! Consistent API migration.All access patterns correctly migrated from pointer-like (
unit->...) to direct value access (unit....).src/Compiler/CompilationUnitImpl.h (1)
15-15: LGTM! Error message storage added.The
error_messagefield appropriately stores error state for the new API'serror()method.src/Index/TUIndex.cpp (3)
78-78: LGTM! Variable naming convention improved.The rename to
relation_rangefollows the codebase's snake_case convention.
93-93: LGTM! Consistent variable usage.Correctly updated to use the renamed
relation_rangevariable.
100-100: LGTM! Consistent variable usage.Correctly updated to use the renamed
relation_rangevariable.tests/unit/Compiler/PreambleTests.cpp (5)
71-71: LGTM! API migration correct.The test correctly uses
unit.success()to verify successful PCH compilation.
86-86: LGTM! Consistent API usage.Correctly verifies successful compilation with PCH using the new API.
187-202: LGTM! Complete API migration.All access patterns correctly migrated from pointer-like (
unit->file_id(),unit->include_location()) to direct value access (unit.file_id(),unit.include_location()). The preprocessing success check is also properly updated.
256-256: LGTM! Consistent test update.Correctly uses the new
success()method in the chain test.
271-271: LGTM! Final verification correct.The final step of the chain test correctly verifies compilation success with the new API.
src/Compiler/CompilationUnit.cpp (2)
28-35: LGTM!The pointer-based access to
src_mgris consistent with the new Impl design.
309-314: Good defensive check added.Adding
include.fid.isValid()before inserting intoall_filesis a sensible guard against invalid file IDs.src/Compiler/Compilation.cpp (5)
200-228: LGTM on the refactoredrun_clangstructure.The refactor cleanly centralizes error handling through
impl->error_message. The early creation ofimplandunitallows consistent error returns throughout the function.
240-242: Consistent error handling pattern.All error paths correctly populate
impl->error_messageand return the unit early. Sinceimpl->instanceremains null on error paths,success()will correctly return false.Also applies to: 258-260, 285-287, 294-298, 303-306
339-348: LGTM on state assignment.The ordering correctly captures the SourceManager pointer before moving
instance. The raw pointer tosrc_mgris safe as it's owned by theCompilerInstancestored inimpl->instance.
353-362: Clean API transition.The public functions now return
CompilationUnitdirectly, aligning with the simplified error-handling model.
415-442: LGTM!The
complete()function correctly adapts to the new return type while maintaining its code-completion setup logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pixi.toml (1)
88-104: Consider movingunit-testtofeature.test.tasksfor consistency.The
unit-testtask (line 88-90) remains under[feature.build.tasks]whileintegration-testandtesthave been moved to[feature.test.tasks]. For organizational clarity and consistency, consider movingunit-testunderfeature.testas well, since all three are test-related tasks.That said, if
unit-testintentionally stays underfeature.buildbecause it only needs build dependencies (not pytest), the current structure is functionally correct since the default environment includes both features.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/publish-clice.ymldocs/en/dev/build.mddocs/zh/dev/build.mdinclude/Compiler/Compilation.hinclude/Server/Server.hpixi.tomlsrc/Compiler/Compilation.cppsrc/Server/Document.cpptests/unit/Test/Tester.cpp
💤 Files with no reviewable changes (1)
- include/Server/Server.h
✅ Files skipped from review due to trivial changes (1)
- docs/zh/dev/build.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/publish-clice.yml
- include/Compiler/Compilation.h
- tests/unit/Test/Tester.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
src/Compiler/Compilation.cpp (2)
src/Compiler/CompilationUnit.cpp (6)
build_at(235-237)build_at(235-235)diagnostics(227-229)diagnostics(227-227)error(24-26)error(24-24)src/Compiler/Directive.cpp (2)
attach(265-268)attach(265-266)
src/Server/Document.cpp (2)
src/Compiler/CompilationUnit.cpp (4)
success(20-22)success(20-20)diagnostics(227-229)diagnostics(227-227)src/Feature/Diagnostic.cpp (2)
diagnostics(9-121)diagnostics(9-9)
🪛 Cppcheck (2.19.0)
src/Server/Document.cpp
[error] 208-208: Uninitialized variable
(uninitvar)
⏰ 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). (11)
- GitHub Check: xmake / build (ubuntu-24.04, debug)
- GitHub Check: xmake / build (ubuntu-24.04, releasedbg)
- GitHub Check: xmake / build (macos-15, releasedbg)
- GitHub Check: xmake / build (windows-2025, releasedbg)
- GitHub Check: cmake / build (windows-2025, RelWithDebInfo)
- GitHub Check: clice / publish-clice (ubuntu-24.04, clice.tar.gz, clice-x86_64-linux-gnu.tar.gz, clice-symbol.tar.gz, cl...
- GitHub Check: xmake / build (macos-15, debug)
- GitHub Check: clice / publish-clice (macos-15, clice.tar.gz, clice-arm64-macos-darwin.tar.gz, clice-symbol.tar.gz, clic...
- GitHub Check: clice / publish-clice (windows-2025, clice.zip, clice-x64-windows-msvc.zip, clice-symbol.zip, clice-x64-w...
- GitHub Check: cmake / build (macos-15, Debug)
- GitHub Check: cmake / build (ubuntu-24.04, Debug)
🔇 Additional comments (12)
docs/en/dev/build.md (1)
33-33: LGTM!The simplified command aligns with the updated
pixi.tomlconfiguration where the default environment now includes bothbuildandtestfeatures, making the explicit-e developflag unnecessary.pixi.toml (3)
1-2: LGTM!The tombi format directive to disable table-keys-order formatting is a reasonable configuration choice that allows maintainers to organize TOML sections in a logical order rather than alphabetical.
19-23: LGTM!Including
testin the default environment ensures developers have access to testing dependencies (pytest, pytest-asyncio) without needing to specify a separate environment. This aligns well with the documentation update to usepixi shellwithout flags.
139-157: LGTM!Placing these helper tasks under the global
[tasks]namespace (rather than feature-specific sections) makes them available across all environments, which is appropriate for utility tasks likebuild-llvm,upload-llvm, anddelete-artifacts.src/Compiler/Compilation.cpp (3)
341-344: Pointer assignment ordering is correct.
impl->src_mgris assigned whileinstanceis still valid, theninstanceis moved intoimpl->instance. TheSourceManagerremains valid since it's owned by theCompilerInstancenow stored inimpl->instance.
352-361: Function signatures consistently updated to returnCompilationUnit.Both
preprocessandcompilenow returnCompilationUnitdirectly, aligning with the new API surface.
200-227: LGTM! The refactored error handling and ownership model looks solid.The function now returns
CompilationUnitdirectly with error state encapsulated inImpl. The flow correctly creates the unit early, allowing all error paths to setimpl->error_messageand return the partially-constructed unit. TheCompilationUnitclass properly takes ownership of the rawimplpointer—the constructor stores it, the move constructor transfers ownership with proper nullification, copy is deleted to prevent double-deletion, and the destructor correctly deletes the pointer.src/Server/Document.cpp (5)
173-179: Signature simplification looks good.Removing the diagnostics parameter in favor of accessing them via
unit.diagnostics()is cleaner and aligns with the unit-based error handling pattern.
208-226: The static analysis warning is a false positive.Cppcheck reports "Uninitialized variable" at line 208, but all captured variables (
params,pch,message,links,path) are properly initialized before the lambda executes. This is likely Cppcheck struggling with coroutine/lambda interactions.The
co_awaitensures the lambda completes before the coroutine continues, so captured references remain valid throughout execution.One minor observation: on line 212,
std::move(unit.error())is redundant sinceerror()returnsstd::stringby value (already an rvalue), but it's harmless.
320-328: LGTM! Consistent use of the new unit-based API.The error handling correctly uses
unit.success(),unit.error(), andunit.diagnostics()matching the refactored pattern.
331-337: Diagnostics generation updated correctly.Using
feature::diagnostics(kind, mapping, unit)aligns with the relevant code snippet showing the function signature expects aCompilationUnit&.
343-343: Move into shared_ptr is appropriate.Creating a
shared_ptrfrom a movedCompilationUnitproperly transfers ownership for shared access to the AST.
16bit-ykiko
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Compiler/CompilationUnit.cpp (1)
46-47: Incorrect assertion condition.Line 47 checks
end.isValid()but the message says "Input source range should be a file range". This should likely checkend.isFileID()instead. Also,begin.isValid()is already checked on line 46, soend.isValid()is redundant there.🔎 Proposed fix
assert(begin.isValid() && end.isValid() && "Invalid source range"); - assert(begin.isFileID() && end.isValid() && "Input source range should be a file range"); + assert(begin.isFileID() && end.isFileID() && "Input source range should be a file range");
♻️ Duplicate comments (1)
src/Compiler/CompilationUnit.cpp (1)
20-26: Null pointer dereference risk remains unaddressed.The past review comment correctly identified that
success()anderror()dereferenceselfwithout null checks. Since the move constructor setsother.self = nullptr(line 209 in header), and the destructor guards withif(self && ...), calling these methods on a moved-from object will crash.🔎 Proposed fix
-bool CompilationUnitRef::success() { - return self->instance != nullptr; +bool CompilationUnitRef::success() const { + return self && self->instance != nullptr; } -std::string CompilationUnitRef::error() { - return self->error_message; +std::string CompilationUnitRef::error() const { + return self ? self->error_message : std::string{}; }
🧹 Nitpick comments (3)
include/Compiler/CompilationUnit.h (2)
41-44: Consider addingconstqualifiers to accessor methods.
success()anderror()are query methods that don't modify state and should be markedconst. This allows calling them on const references and communicates intent.🔎 Proposed fix
public: - bool success(); + bool success() const; - std::string error(); + std::string error() const;
204-204: Reorder member initializer list to match initialization order.Base classes are initialized before members in C++. The current order (
kind(kind), CompilationUnitRef(impl)) is misleading and will trigger-Wreorderwarnings on most compilers.🔎 Proposed fix
- CompilationUnit(Kind kind, Self* impl) : kind(kind), CompilationUnitRef(impl) {} + CompilationUnit(Kind kind, Self* impl) : CompilationUnitRef(impl), kind(kind) {}src/Compiler/CompilationUnit.cpp (1)
269-271: Variable shadowing reduces readability.The loop variable
depsshadows the outerdepsStringSet. While this works, it's confusing and error-prone if the code is later modified.🔎 Proposed fix
- for(auto& deps: deps) { - result.emplace_back(deps.getKey().str()); + for(auto& entry: deps) { + result.emplace_back(entry.getKey().str()); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
include/Compiler/CompilationUnit.hinclude/Compiler/Diagnostic.hinclude/Compiler/Tidy.hsrc/Compiler/Compilation.cppsrc/Compiler/CompilationUnit.cppsrc/Compiler/CompilationUnitImpl.hsrc/Compiler/Diagnostic.cppsrc/Compiler/Implement.hsrc/Compiler/Tidy.cppsrc/Compiler/TidyImpl.htests/unit/Compiler/TidyTests.cpp
💤 Files with no reviewable changes (3)
- src/Compiler/TidyImpl.h
- src/Compiler/CompilationUnitImpl.h
- include/Compiler/Tidy.h
🧰 Additional context used
🧬 Code graph analysis (3)
include/Compiler/CompilationUnit.h (2)
src/Compiler/CompilationUnit.cpp (7)
success(20-22)success(20-20)error(24-26)error(24-24)diagnostics(228-230)diagnostics(228-228)CompilationUnit(7-18)src/Compiler/Implement.h (1)
Self(66-109)
src/Compiler/Compilation.cpp (3)
src/Compiler/CompilationUnit.cpp (4)
build_at(236-238)build_at(236-236)error(24-26)error(24-24)src/Compiler/Diagnostic.cpp (2)
create_diagnostic(259-261)create_diagnostic(259-259)src/Compiler/Directive.cpp (2)
attach(265-268)attach(265-266)
src/Compiler/CompilationUnit.cpp (1)
include/Compiler/CompilationUnit.h (1)
interested_content(83-216)
⏰ 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). (11)
- GitHub Check: cmake / build (macos-15, Debug)
- GitHub Check: xmake / build (macos-15, releasedbg)
- GitHub Check: clice / publish-clice (macos-15, clice.tar.gz, clice-arm64-macos-darwin.tar.gz, clice-symbol.tar.gz, clic...
- GitHub Check: xmake / build (ubuntu-24.04, debug)
- GitHub Check: xmake / build (ubuntu-24.04, releasedbg)
- GitHub Check: clice / publish-clice (ubuntu-24.04, clice.tar.gz, clice-x86_64-linux-gnu.tar.gz, clice-symbol.tar.gz, cl...
- GitHub Check: xmake / build (macos-15, debug)
- GitHub Check: clice / publish-clice (windows-2025, clice.zip, clice-x64-windows-msvc.zip, clice-symbol.zip, clice-x64-w...
- GitHub Check: xmake / build (windows-2025, releasedbg)
- GitHub Check: cmake / build (windows-2025, RelWithDebInfo)
- GitHub Check: cmake / build (ubuntu-24.04, Debug)
🔇 Additional comments (18)
include/Compiler/Diagnostic.h (1)
12-13: LGTM: Clean forward declaration.The forward declaration reduces header coupling and aligns with the refactored diagnostic handling that now uses
CompilationUnitRef.src/Compiler/Tidy.cpp (1)
1-1: LGTM: Header consolidation.The include refactoring to
Implement.haligns with the broader reorganization of the compilation unit implementation surface.tests/unit/Compiler/TidyTests.cpp (1)
25-26: LGTM: Correct API usage.The changes correctly use the new
CompilationUnitAPI withsuccess()to check compilation status and direct access todiagnostics()instead of the previous optional-style access.src/Compiler/Diagnostic.cpp (4)
3-3: LGTM: Header consolidation.Consistent with the refactoring to consolidate implementation headers into
Implement.h.
206-215: LGTM: Improved ownership model.The refactoring from shared pointer ownership to
CompilationUnitRef-based storage provides clearer ownership semantics and aligns with the new ref-driven diagnostic collection model.
254-254: LGTM: Member type updated consistently.The private member type change aligns with the constructor refactoring and the new diagnostic ownership model.
259-261: LGTM: Factory function signature updated.The factory function signature correctly reflects the new
CompilationUnitRef-based API.src/Compiler/Compilation.cpp (6)
3-3: LGTM: Header consolidation.Consistent with the refactoring to consolidate implementation details into
Implement.h.
221-222: LGTM: Consistent error handling pattern.The error handling consistently sets
self->error_messageand returns the unit object instead of usingstd::unexpected. This aligns well with the new API whereunit.success()checks compilation status andunit.error()retrieves error messages.Also applies to: 235-236, 253-254, 280-281, 291-292, 299-300
248-249: LGTM: Centralized state management.Moving all compilation state (top-level declarations, directives, buffer, resolver, build timing, and instance) into the
Selfobject provides a clean, centralized state management approach that can be accessed throughCompilationUnitRef.Also applies to: 271-271, 304-304, 311-311, 324-324, 333-339
208-208: LGTM: Updated diagnostic creation.The call to
create_diagnostic(self)correctly uses the refactored API where diagnostics are collected directly into theCompilationUnitRefinstead of a separate shared vector.
347-407: LGTM: Public API refactored consistently.All public compilation functions (
preprocess,compileoverloads, andcomplete) now returnCompilationUnitinstead of a result-style type. This provides a cleaner, value-based API where callers usesuccess()anderror()accessors to check compilation status.
198-207: No action needed. TheCompilationUnitclass properly manages theSelf*pointer through RAII: the constructor takes ownership, the destructor explicitly deletes it (line 17 in src/Compiler/CompilationUnit.cpp), and move semantics are correctly implemented to prevent double deletion.include/Compiler/CompilationUnit.h (1)
206-210: LGTM!The deleted copy constructor and move constructor correctly implement move-only semantics, properly transferring ownership of
selfand nullifying the source to prevent double-deletion.src/Compiler/CompilationUnit.cpp (2)
7-18: LGTM!The destructor correctly guards against null
self, properly manages the preprocessor lifecycle by detaching beforeEndSourceFile(), and ensures cleanup withdelete self.
328-331: LGTM!The assertion correctly validates that the
TemplateResolveris available before dereferencing. Similar defensive checks are consistently applied across other optional members.src/Compiler/Implement.h (2)
27-41: LGTM!The
ClangTidyCheckerclass properly encapsulates the clang-tidy integration components. Public members are acceptable here as this appears to be a configuration/state holder used internally.
111-116: LGTM!The
DiagnosticCollectorfollows a clean factory pattern withcreate_diagnostic(). The class properly inherits fromclang::DiagnosticConsumerand maintains a pointer to the checker for adjusting diagnostic levels.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
include/Compiler/CompilationUnit.h (1)
37-37: Empty enumCompilationStatusappears unused.This enum is declared but has no enumerators and is not referenced elsewhere in this file. If this is intended for future use, consider adding a
TODOcomment explaining its purpose. Otherwise, it may be dead code.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
include/Compiler/Compilation.hinclude/Compiler/CompilationUnit.hinclude/Compiler/Diagnostic.hsrc/Compiler/Compilation.cppsrc/Server/Document.cppsrc/Server/Feature.cppsrc/Server/Indexer.cpptests/unit/Test/Tester.cpp
💤 Files with no reviewable changes (1)
- include/Compiler/Diagnostic.h
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/Test/Tester.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
src/Server/Document.cpp (2)
src/Compiler/CompilationUnit.cpp (4)
success(20-22)success(20-20)diagnostics(228-230)diagnostics(228-228)src/Feature/Diagnostic.cpp (2)
diagnostics(9-121)diagnostics(9-9)
src/Server/Indexer.cpp (1)
src/Index/TUIndex.cpp (2)
build(169-177)build(169-169)
🪛 Cppcheck (2.19.0)
src/Server/Document.cpp
[error] 208-208: Uninitialized variable
(uninitvar)
[error] 312-312: Found an exit path from function with non-void return type that has missing return statement
(missingReturn)
⏰ 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). (11)
- GitHub Check: xmake / build (macos-15, debug)
- GitHub Check: xmake / build (ubuntu-24.04, releasedbg)
- GitHub Check: xmake / build (macos-15, releasedbg)
- GitHub Check: xmake / build (ubuntu-24.04, debug)
- GitHub Check: clice / publish-clice (ubuntu-24.04, clice.tar.gz, clice-x86_64-linux-gnu.tar.gz, clice-symbol.tar.gz, cl...
- GitHub Check: clice / publish-clice (macos-15, clice.tar.gz, clice-arm64-macos-darwin.tar.gz, clice-symbol.tar.gz, clic...
- GitHub Check: cmake / build (windows-2025, RelWithDebInfo)
- GitHub Check: cmake / build (macos-15, Debug)
- GitHub Check: xmake / build (windows-2025, releasedbg)
- GitHub Check: cmake / build (ubuntu-24.04, Debug)
- GitHub Check: clice / publish-clice (windows-2025, clice.zip, clice-x64-windows-msvc.zip, clice-symbol.zip, clice-x64-w...
🔇 Additional comments (17)
src/Compiler/Compilation.cpp (4)
198-223: LGTM on the refactoredrun_clangentry and error handling.The function correctly initializes
CompilationUnitRef::Selfvia raw allocation and wraps it inCompilationUnit. Error cases consistently populateself->error_messagebefore returning the unit. The ownership transfer is handled properly via theCompilationUnitconstructor.
245-255: LGTM on ProxyAction setup and source file handling.The conditional for collecting top-level declarations correctly checks both
clang_tidyandCompilationKind::Content, and error paths consistently setself->error_messagebefore returning.
332-343: LGTM on final state population.Build timing metrics, source manager references, and action/instance ownership are properly transferred to
selfbefore returning the unit.
347-356: LGTM on public API wrappers.
preprocessandcompilecorrectly delegate torun_clangwith the appropriate action types and returnCompilationUnitconsistently.include/Compiler/CompilationUnit.h (2)
199-209: LGTM on theCompilationUnitclass design.The move constructor correctly transfers ownership by moving
selfand nullifying the source's pointer. The deleted copy constructor prevents accidental copies. The member initializer order in the primary constructor (line 201) is correct since base classes are always initialized before members regardless of the textual order.
189-196: LGTM on theCompilationUnitRefdesign.The forward-declared
Selfstruct with protected pointer member provides a clean separation between the public interface and implementation details.src/Server/Feature.cpp (2)
32-38: LGTM onCompilationKindenum migration.The change from
CompilationUnit::CompletiontoCompilationKind::Completioncorrectly aligns with the refactored enum location.
90-96: LGTM on signature help enum update.Consistent with the enum migration pattern applied in
on_completion.src/Server/Indexer.cpp (2)
11-14: LGTM on compilation params setup.The enum migration to
CompilationKind::Indexingand the addition of.query_toolchain = trueare consistent with the broader refactor. The designated initializer syntax is clean.
26-34: LGTM on unit-based result handling.The change from truthiness check to
unit.success()and fromTUIndex::build(*unit)toTUIndex::build(unit)correctly aligns with the newCompilationUnitvalue-based API.include/Compiler/Compilation.h (2)
16-18: LGTM onCompilationParamsupdate.The
kindmember type migration toCompilationKindis consistent with the enum relocation.
62-77: LGTM on API return type changes.The migration from
CompilationResult(expected-style) toCompilationUnit(value-style withsuccess()/error()accessors) simplifies the public API surface. All function signatures are updated consistently.src/Server/Document.cpp (5)
174-179: LGTM on signature update forbuild_pch_task.The removal of the diagnostics parameter aligns with the unit-based diagnostics model where diagnostics are accessed via
unit.diagnostics()instead of being passed as a separate parameter.
208-226: LGTM on PCH build error handling.The lambda correctly uses the unit-based API:
unit.success()for status check,unit.error()for the error message, andunit.diagnostics()for iterating through diagnostics. Themessagevariable referenced by cppcheck is initialized on line 205 before the lambda, so the static analysis warning is a false positive.
320-328: LGTM on AST build error handling.Consistent with the PCH build path, using
unit.success(),unit.error(), andunit.diagnostics()for error reporting. The static analysis warning about a missing return at line 312 is a false positive—this is a coroutine function with properco_returnstatements.
331-343: LGTM on diagnostics publishing and AST storage.The diagnostics are correctly generated via
feature::diagnostics(kind, mapping, unit)and the AST is stored asstd::make_shared<CompilationUnit>(std::move(unit)), properly transferring ownership.
277-278: LGTM on task creation update.The
build_pch_taskcall correctly omits the old diagnostics parameter and moves content ownership into the task.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Compiler/CompilationUnit.cpp (1)
11-324: Establish class invariant for self pointer.Every method in this file unconditionally dereferences
selfwithout null checks. You have two options:
Establish a class invariant: Document that
selfmust never be null except in moved-from objects, and mark moved-from objects as unusable (either through assertions or by making methods checksuccess()first).Add null checks everywhere: Check
selfin every method, which would be verbose but safer.Given the design, option 1 seems more appropriate. Consider:
- Adding
assert(self)at the start of methods that require valid state- Documenting that methods must not be called on moved-from objects
- Making the move constructor private or documenting the invariant clearly
♻️ Duplicate comments (2)
src/Compiler/CompilationUnit.cpp (1)
7-9: Critical: Null pointer dereference risk in success().This issue was flagged in a previous review but remains unaddressed. The method dereferences
selfwithout a null check. Since the move constructor (line 205-207 in the header) setsother.self = nullptr, callingsuccess()on a moved-from object will crash.Additionally, this method should be
constsince it doesn't modify state.🔎 Proposed fix
-bool CompilationUnitRef::success() { +bool CompilationUnitRef::success() const { + return self && self->instance != nullptr; - return self->instance != nullptr; }Also update the declaration in
include/Compiler/CompilationUnit.h.Based on past review comments.
src/Compiler/Implement.h (1)
20-20: Document the intended use of the emptyTidyParamsstruct.
TidyParamsis currently empty but is passed toconfigure()andconfigure_tidy(). Document whether this is a placeholder for future extension or if parameters will be added.
🧹 Nitpick comments (3)
include/Compiler/CompilationUnit.h (2)
37-37: Unused enum declaration.
CompilationStatusis declared but never used in the codebase. Consider removing it or documenting its intended future use.🔎 Verify usage across the codebase
#!/bin/bash # Search for any usage of CompilationStatus rg -n "CompilationStatus" --type=cpp --type=h
163-163: Consider providing a const overload for diagnostics().Returning a mutable reference prevents calling this method on const
CompilationUnitRefobjects. While the current non-const version is needed for building diagnostics, consider adding a const overload for read-only access:auto diagnostics() const -> const std::vector<Diagnostic>&;This would improve const-correctness throughout the API.
src/Compiler/Implement.h (1)
107-107: Resolve or track the TODO comment.The TODO questions whether modifying the unit-level traversal scope is acceptable. This design decision should be resolved and documented, or tracked as a follow-up issue.
Do you want me to open a new issue to track this TODO?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignoreinclude/Compiler/CompilationUnit.hinclude/Compiler/Directive.hsrc/Compiler/Compilation.cppsrc/Compiler/CompilationUnit.cppsrc/Compiler/Diagnostic.cppsrc/Compiler/Directive.cppsrc/Compiler/Implement.h
💤 Files with no reviewable changes (1)
- include/Compiler/Directive.h
🧰 Additional context used
🧬 Code graph analysis (3)
src/Compiler/Compilation.cpp (2)
src/Compiler/Diagnostic.cpp (2)
create_diagnostic(252-254)create_diagnostic(252-252)src/Compiler/CompilationUnit.cpp (3)
success(7-9)success(7-7)CompilationUnit(322-324)
src/Compiler/Directive.cpp (1)
src/Compiler/Compilation.cpp (2)
instance(79-86)instance(79-80)
src/Compiler/Diagnostic.cpp (2)
tests/unit/Test/Tester.cpp (2)
range(138-151)range(138-138)include/AST/SourceCode.h (1)
range(71-95)
⏰ 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). (11)
- GitHub Check: xmake / build (windows-2025, releasedbg)
- GitHub Check: cmake / build (ubuntu-24.04, Debug)
- GitHub Check: xmake / build (macos-15, debug)
- GitHub Check: xmake / build (ubuntu-24.04, releasedbg)
- GitHub Check: xmake / build (ubuntu-24.04, debug)
- GitHub Check: xmake / build (macos-15, releasedbg)
- GitHub Check: clice / publish-clice (macos-15, clice.tar.gz, clice-arm64-macos-darwin.tar.gz, clice-symbol.tar.gz, clic...
- GitHub Check: cmake / build (windows-2025, RelWithDebInfo)
- GitHub Check: clice / publish-clice (windows-2025, clice.zip, clice-x64-windows-msvc.zip, clice-symbol.zip, clice-x64-w...
- GitHub Check: cmake / build (macos-15, Debug)
- GitHub Check: clice / publish-clice (ubuntu-24.04, clice.tar.gz, clice-x86_64-linux-gnu.tar.gz, clice-symbol.tar.gz, cl...
🔇 Additional comments (8)
.gitignore (1)
63-63: LGTM!Adding
.clangdto the IDE/Editors section is appropriate—it ignores the clang language server cache directory. Placement and format are consistent with surrounding entries.src/Compiler/Directive.cpp (1)
267-270: LGTM - Clean refactoring to unit-centric design.The migration from a free function to a member function of
CompilationUnitRef::Selfproperly encapsulates directive collection within the compilation unit's lifecycle. The Preprocessor is now obtained from the unit's instance rather than passed as a parameter, which aligns well with the overall refactoring goals.src/Compiler/Diagnostic.cpp (2)
153-203: LGTM - Improved encapsulation of diagnostic range computation.The refactoring of
diagnostic_rangeinto a member function ofDiagnosticCollectorthat uses the unit's context is a good design improvement. This eliminates the need to passLangOptionsas a parameter and ensures consistent access to location information through the unit's interface.
207-244: Add null safety guard to CompilationUnitRef::operator->().The
operator->()returnsselfdirectly without null validation. Since move semantics allowselfto be nullptr, this creates undefined behavior risk. Add a runtime check or assert inoperator->(), or alternatively add a null check guard before any use ofunit->inHandleDiagnostic(lines 213, 238). At minimum, ensure the DiagnosticCollector constructor validates thatunit.selfis not null.src/Compiler/Compilation.cpp (2)
140-148: Good practice: Explicit memory management with clear ownership.Setting
RetainRemappedFileBuffers = trueand transferring ownership toself.remapped_buffersensures deterministic cleanup. The comment clearly explains why this is necessary (CompilerInstance doesn't reliably clear these buffers, especially on early abort).
161-177: Essential safeguard: Disabling side-effect output options.Explicitly disabling help, stats, version, time-trace, and other output options is critical for LSP operation. These outputs would corrupt the JSON-RPC transport if written to stdout/stderr. Well done.
src/Compiler/CompilationUnit.cpp (1)
322-324: LGTM - Proper cleanup in destructor.The destructor correctly deletes
self, completing the ownership pattern established inCompilation.cppwhereSelf*is allocated withnewand transferred toCompilationUnit.src/Compiler/Implement.h (1)
119-128: Destructor logic is correctly implemented and well-documented.The pattern of calling
instance->getPreprocessor().EndSourceFile()inrun_tidy()(line 115) and then detaching the preprocessor beforeaction->EndSourceFile()in the destructor (lines 125-126) is intentional design, not fragility. The code explicitly documents the clang lifecycle requirements:
run_tidy()already notifies the preprocessor of EOF to flush diagnostics without destroying ASTContext (lines 112-115)- The destructor keeps the preprocessor alive via
shared_ptrto satisfy Sema's requirements duringEndSourceFile()(line 123)- Detaching the preprocessor (line 125) prevents double EOF notification
- The ordering constraint is explained in the comments (lines 103, 121-122)
The same pattern appears consistently in Compilation.cpp where
EndSourceFile()is called early in error paths before action reset. This is a well-reasoned approach to clang's documented lifecycle, not an undocumented fragile dependency.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/Compiler/Implement.h (2)
14-14: Avoidusing namespacein header files.This issue was already flagged in a previous review. The
using namespace clang::tidy;directive in a header pollutes the global namespace for all translation units that include this file.
75-76: Typo in comment.This was already noted in a previous review: "diretive" should be "directive".
src/Compiler/Compilation.cpp (1)
318-339: Exception safety concern with rawnew- previously flagged.The raw
newat line 322 without RAII protection was already identified in a past review. If an exception occurs before line 338, theSelf*will leak. Consider usingstd::unique_ptras suggested in the previous review.src/Compiler/CompilationUnit.cpp (1)
7-13: Accessors should haveconstqualifiers.The
kind()andstatus()methods are read-only accessors and should be markedconst. This was flagged in a previous review.
🧹 Nitpick comments (2)
src/Server/Document.cpp (1)
318-326: Consider enabling diagnostic logging on AST build failure.The error logging at lines 321-324 is commented out. Unlike PCH builds where diagnostics are logged (lines 214-216), AST build failures silently return without any diagnostic information. Consider enabling similar logging for consistency and debugging purposes.
if(!unit.completed()) { /// FIXME: Fails needs cancel waiting tasks. - /// LOG_ERROR("Building AST fails for {}, Beacuse: {}", path, unit.error()); - /// for(auto& diagnostic: unit.diagnostics()) { - /// LOG_ERROR("{}", diagnostic.message); - /// } + LOG_ERROR("Building AST fails for {}", path); + for(auto& diagnostic: unit.diagnostics()) { + LOG_ERROR("{}", diagnostic.message); + } co_return; }include/Compiler/CompilationUnit.h (1)
44-79: Consider addingconstqualifiers to status query methods.The status query methods (
kind(),status(),completed(),cancelled(),setup_fail(),fatal_error()) don't modify state and should beconstfor proper const-correctness. This enables calling these methods on const references.🔎 Proposed fix
- CompilationKind kind(); + CompilationKind kind() const; - CompilationStatus status(); + CompilationStatus status() const; /// Parse finished; ASTContext is usable but diagnostics may still contain errors. - bool completed() { + bool completed() const { return status() == CompilationStatus::Completed; } /// Compilation was cancelled; consumers should not touch any state. - bool cancelled() { + bool cancelled() const { return status() == CompilationStatus::Cancelled; } /// Failed during initial setup; diagnostics exist (location-free), ASTContext /// is unavailable. - bool setup_fail() { + bool setup_fail() const { return status() == CompilationStatus::SetupFail; } /// Hit an unrecoverable error; diagnostics and decoded source locations /// are usable, other states are not unavailable. - bool fatal_error() { + bool fatal_error() const { return status() == CompilationStatus::FatalError; }Based on past review comments highlighting const-correctness issues.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
include/Compiler/CompilationUnit.hsrc/Compiler/Compilation.cppsrc/Compiler/CompilationUnit.cppsrc/Compiler/Implement.hsrc/Compiler/Module.cppsrc/Feature/CodeCompletion.cppsrc/Feature/SignatureHelp.cppsrc/Server/Document.cppsrc/Server/Indexer.cpptests/unit/Compiler/DiagnosticTests.cpptests/unit/Compiler/ModuleTests.cpptests/unit/Compiler/PreambleTests.cpptests/unit/Compiler/TidyTests.cpptests/unit/Compiler/ToolchainTests.cpptests/unit/Test/Tester.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/Compiler/TidyTests.cpp
- src/Server/Indexer.cpp
- tests/unit/Compiler/PreambleTests.cpp
- tests/unit/Test/Tester.cpp
🧰 Additional context used
🧬 Code graph analysis (6)
src/Feature/CodeCompletion.cpp (1)
src/Compiler/Compilation.cpp (2)
complete(409-438)complete(409-409)
src/Compiler/Compilation.cpp (2)
src/Compiler/Diagnostic.cpp (2)
create_diagnostic(252-254)create_diagnostic(252-252)src/Compiler/CompilationUnit.cpp (1)
CompilationUnit(326-328)
include/Compiler/CompilationUnit.h (2)
src/Compiler/Implement.h (1)
Self(55-99)src/Compiler/CompilationUnit.cpp (7)
kind(7-9)kind(7-7)status(11-13)status(11-11)diagnostics(213-215)diagnostics(213-213)CompilationUnit(326-328)
src/Compiler/CompilationUnit.cpp (2)
src/Feature/SemanticToken.cpp (11)
location(93-116)location(93-93)fid(73-85)fid(88-91)fid(88-88)fid(119-202)fid(119-119)tokens(214-240)tokens(214-214)decl(17-35)decl(17-19)include/Compiler/CompilationUnit.h (1)
interested_content(118-244)
src/Compiler/Implement.h (3)
include/Compiler/Directive.h (1)
clice(8-140)src/Compiler/Tidy.cpp (9)
is_registered_tidy_check(23-40)is_registered_tidy_check(23-23)is_fast_tidy_check(42-54)is_fast_tidy_check(42-42)ClangTidyChecker(250-251)adjust_level(253-291)adjust_level(254-255)adjust_diag(293-315)adjust_diag(293-293)include/Compiler/CompilationUnit.h (1)
CompilationUnitRef(44-246)
src/Feature/SignatureHelp.cpp (1)
src/Compiler/Compilation.cpp (2)
complete(409-438)complete(409-409)
🪛 Cppcheck (2.19.0)
src/Server/Document.cpp
[error] 208-208: Uninitialized variable
(uninitvar)
[error] 312-312: Found an exit path from function with non-void return type that has missing return statement
(missingReturn)
🔇 Additional comments (20)
src/Feature/SignatureHelp.cpp (1)
206-209: LGTM!The migration to the new
CompilationUnitAPI is correct. The code now checksunit.completed()instead of relying on the previous truthiness-based pattern, which is clearer and more explicit about the unit's state.src/Feature/CodeCompletion.cpp (1)
435-438: LGTM!The change correctly adopts the new
CompilationUnitAPI pattern, checkingunit.completed()for success. This is consistent with the signature help implementation and the broader refactoring.tests/unit/Compiler/ToolchainTests.cpp (1)
76-77: LGTM!The test updates correctly reflect the new
CompilationUnitAPI:
unit.completed()replaces the previous optional/pointer-style checkunit.diagnostics()uses direct value access instead of pointer dereferenceThese changes are consistent across both GCC and Clang test cases.
Also applies to: 115-116
src/Compiler/Implement.h (1)
108-121: The TODO about traversal scope side effects remains valid and unresolved.Line 111 correctly raises a concern:
setTraversalScope()modifies theASTContextto restrict traversal totop_level_declsbut is never reset. The scope remains restricted when theTemplateResolveris instantiated immediately after at line 310 inCompilation.cpp. Whether this affects the resolver's ability to perform semantic resolution depends on Clang's API semantics, which cannot be determined from code inspection alone. The concern merits either clarification in a follow-up comment or explicit scope restoration before resolver instantiation.tests/unit/Compiler/ModuleTests.cpp (1)
29-33: LGTM!The API migration from boolean result to
unit.completed()is correct and aligns with the new unit-centric compilation flow.src/Server/Document.cpp (3)
208-224: Static analysis: potential uninitialized variable warning is a false positive.The static analyzer flagged line 208 for an uninitialized variable, but all captured references (
params,pch,message,links,path) are properly initialized before the lambda is invoked viaasync::submit. Thepathparameter at line 177 is initialized from the function argument.The code correctly handles the new unit-based API flow.
211-223: Diagnostic handling during PCH build is incomplete but tracked.The FIXME at line 212 correctly identifies that diagnostics should be flushed through
feature::diagnosticfor proper LSP reporting. The current logging approach is acceptable for debugging but won't surface errors to the user through the LSP protocol.
341-341: LGTM!Correct use of move semantics to transfer ownership of the
CompilationUnitinto ashared_ptrfor storage.src/Compiler/Compilation.cpp (3)
140-149: Good practice: explicit buffer lifetime management.The comment at lines 141-143 clearly explains why
RetainRemappedFileBuffersis set to true and that cleanup happens inCompilationUnit's destructor. This is a sensible approach to handle the non-deterministic cleanup behavior ofCompilerInstance.
208-316: Well-structured compilation flow with proper status handling.The internal
run_clangfunction has clear status transitions:
SetupFailfor early configuration failures (diagnostic engine, invocation, target, action begin)FatalErrorfor execution errors or PCH/PCM generation failuresCancelledwhen the stop flag is setCompletedon successThe error handling at lines 290-295 correctly prevents serializing a potentially corrupt AST when generating PCH/PCM files.
343-438: LGTM! Consistent public API returning CompilationUnit.All public entry points (
preprocess,compileoverloads,complete) now consistently returnCompilationUnit, enabling callers to query completion status and access diagnostics uniformly. Thebefore_executeandafter_executecallbacks are well-organized for different compilation modes.tests/unit/Compiler/DiagnosticTests.cpp (4)
83-99: Good test coverage for setup failure scenario.The
TargetErrortest correctly validates:
setup_fail()returns true for invalid target triple- Diagnostics are still accessible despite setup failure
- Diagnostic has no valid file location (as expected for command-line errors)
The test appropriately uses an unknown target triple
aa-bb-ccto trigger the setup failure path.
101-117: LGTM! Error test correctly validates completed state with errors.The test confirms that
completed()returns true even when parse errors occur, which is the expected behavior - the AST is built (though potentially incomplete) and diagnostics are available.
119-134: Good addition: Warning diagnostic test.This new test case validates warning-level diagnostics and confirms the diagnostic collection works for non-error diagnostics. The use of
-Wall -Wunused-variableflags is appropriate.
136-163: LGTM! PCHError and ASTError tests validate error state differentiation.
PCHError: Correctly expectsfatal_error()because PCH generation is aborted when errors occur (to prevent corrupted PCH files)ASTError: Correctly expectscompleted()for the same code without PCH output, as the AST can still be used despite errorsThis demonstrates the important distinction between PCH/PCM generation (which must abort on errors) and regular AST building (which can tolerate errors).
include/Compiler/CompilationUnit.h (2)
15-42: Well-designed enum types for compilation state.The
CompilationKindenum clearly documents each compilation mode with helpful comments. TheCompilationStatusenum provides a clean state machine for tracking compilation outcomes.
230-244: LGTM! Proper ownership semantics in CompilationUnit.The move constructor correctly transfers ownership by nullifying the source's
selfpointer. Combined with the deleted copy constructor and the destructor that deletesself, this provides correct RAII semantics.Note: The
kindmember at line 243 shadows the base class methodkind()at line 55. This appears intentional as the derived class stores the kind directly while the base accesses it throughself. Consider whether this duplication is necessary.src/Compiler/CompilationUnit.cpp (3)
79-107: LGTM! Efficient file path caching implementation.The path caching logic correctly:
- Checks the cache first
- Retrieves the real path (with fallback to virtual path)
- Allocates stable storage via
BumpPtrAllocator- Stores in cache for future lookups
The null-terminated allocation (line 102) ensures C-string compatibility.
261-288: LGTM! Symbol ID generation with caching.Both
getSymbolIDimplementations follow the same efficient pattern:
- Check cache first
- Generate USR on cache miss
- Compute hash and cache it
- Return SymbolID with hash and name
The caching prevents redundant USR generation for frequently accessed symbols.
326-328: Destructor correctly manages Self ownership.The destructor simply deletes the
selfpointer. SinceCompilationUnitis the sole owner (move constructor nullifies source), this is correct. The deleted copy constructor prevents double-delete scenarios.Note: Consider adding a null check for safety if a moved-from unit is accidentally destroyed twice (though this shouldn't happen with proper usage):
CompilationUnit::~CompilationUnit() { delete self; // delete nullptr is safe, so this is fine as-is }Actually,
delete nullptris well-defined and safe in C++, so the current implementation is correct even for moved-from objects.
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
1 participant
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.