Repository navigation
build: update llvm to 21.1.4 - #292
Conversation
WalkthroughCMake now requires an explicit LLVM_VERSION and generalizes clang copy/install paths; prebuilt LLVM assets bumped to 21.1.4. Diagnostics factory returns std::unique_ptr and compilation wiring updated for ownership. Dependent-template lookup is name-guarded; resource dir paths move to lib/clang/21 and related tidy/index/compiler tweaks applied. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CMake as cmake/llvm_setup.cmake
participant Remote as prebuilt-llvm.json
participant FS as Filesystem/install
Dev->>CMake: setup_llvm(LLVM_VERSION)
CMake->>Remote: lookup assets for LLVM_VERSION
Remote-->>CMake: asset metadata
CMake->>CMake: fetch/install archive
CMake->>FS: copy/install `clang/*` -> `lib/clang/` (generalized)
CMake-->>Dev: LLVM configured for LLVM_VERSION
sequenceDiagram
participant Caller
participant Compilation as src/Compiler/Compilation.cpp
participant DiagFactory as Diagnostic::create
participant DiagConsumer as DiagnosticCollector (unique_ptr)
participant Instance as CompilerInstance
Caller->>Compilation: prepare diagnostics vector
Compilation->>DiagFactory: Diagnostic::create(diagnostics)
DiagFactory-->>DiagConsumer: returns std::unique_ptr
rect rgb(235,245,255)
Note over DiagConsumer,Instance: ownership transfer via release()/move
end
Compilation->>Instance: createDiagnostics(..., DiagConsumer.release())
Compilation->>Compilation: std::move(diagnostics) into unit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
🔇 Additional comments (2)
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 |
168783c to
0fa00b5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
config/prebuilt-llvm.json (1)
11-17: Release-LTO archives still pinned to 20.1.5We now hard-pin the toolchain to LLVM 21.1.4 and resolve the resource directory under
lib/clang/21. However, all of theis_ltoentries here still reference the 20.1.5 artifacts. When the release/LTO builds fetch those bundles they only containlib/clang/20, sofs::real_pathininit_resource_diraborts because the expectedlib/clang/21directory is missing.(reviews.llvm.org) With the 21.1.4 release already available, the LTO archives need to be rebuilt (and the version/filename fields bumped) before this upgrade can ship, otherwise every release+LTO configuration will fail to start.(discourse.llvm.org)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
CMakeLists.txt(1 hunks)cmake/llvm_setup.cmake(6 hunks)cmake/package.cmake(1 hunks)config/prebuilt-llvm.json(3 hunks)include/AST/Resolver.h(1 hunks)include/AST/Semantic.h(0 hunks)include/Compiler/Diagnostic.h(1 hunks)include/Support/FileSystem.h(1 hunks)src/AST/Resolver.cpp(6 hunks)src/Compiler/Compilation.cpp(4 hunks)src/Compiler/Diagnostic.cpp(1 hunks)src/Compiler/Directive.cpp(1 hunks)src/Compiler/Tidy.cpp(3 hunks)src/Feature/CodeCompletion.cpp(1 hunks)src/Index/USRGeneration.cpp(3 hunks)tests/unit/Compiler/Tidy.cpp(1 hunks)xmake.lua(1 hunks)
💤 Files with no reviewable changes (1)
- include/AST/Semantic.h
🧰 Additional context used
🧬 Code graph analysis (5)
include/AST/Resolver.h (1)
src/AST/Resolver.cpp (8)
type(308-354)type(308-308)type(467-561)type(467-467)type(643-653)type(643-643)lookup(801-805)lookup(801-802)
src/Compiler/Tidy.cpp (1)
src/Compiler/Compilation.cpp (2)
instance(80-87)instance(80-81)
tests/unit/Compiler/Tidy.cpp (2)
src/Compiler/Tidy.cpp (2)
is_fast_tidy_check(54-66)is_fast_tidy_check(54-54)src/Compiler/Compilation.cpp (6)
compile(317-319)compile(317-317)compile(321-348)compile(321-321)compile(350-370)compile(350-350)
src/AST/Resolver.cpp (1)
include/AST/Resolver.h (4)
lookup(43-45)lookup(47-56)lookup(58-60)lookup(62-97)
src/Compiler/Compilation.cpp (2)
src/Compiler/Diagnostic.cpp (2)
create(260-263)create(261-261)src/Compiler/CompilationUnit.cpp (2)
diagnostics(217-219)diagnostics(217-217)
⏰ 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: linux (ubuntu-24.04, release)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: macos (macos-15, release)
- GitHub Check: windows (windows-2025)
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
🔇 Additional comments (2)
src/Compiler/Directive.cpp (1)
126-128: API usage is correct; no null safety concerns.The code properly adapts to LLVM 21.1.4's ModuleIdPath iteration pattern where IdentifierInfo pointers are always valid for identifier tokens. The change from structured binding to explicit method calls (
getIdentifierInfo()andgetLoc()) correctly maps to the LLVM 21.1.4 IdentifierLoc API. Similar patterns appear in Clang's pragma handling code without null checks, confirming this is standard usage for module import paths where identifier information is guaranteed to be present.src/Feature/CodeCompletion.cpp (1)
184-186: Code is correct and complete—no missing Decl kinds.The three new Decl kinds added in lines 184-186 (OpenACCRoutine, OpenACCDeclare, HLSLRootSignature) are properly handled. Verification confirms that all other new Clang 21 Decl kinds (UsingPack, UnresolvedUsingIfExists, PragmaComment, PragmaDetectMismatch, etc.) are already accounted for elsewhere in the switch statement. The three additions are correctly categorized as non-NamedDecl types that cannot appear in code completion results and appropriately marked with std::unreachable(). The switch statement is exhaustive with no missing enum cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
include/Support/FileSystem.h (1)
47-47: Consider usingerror.message()for more readable error messages.Formatting
std::error_codedirectly produces output like "system:2". Usingerror.message()would provide a more user-friendly error description.Apply this diff:
- return std::unexpected(std::format("{}:{}", error, path)); + return std::unexpected(std::format("{}:{}", error.message(), path));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
include/Support/FileSystem.h(1 hunks)
🔇 Additional comments (2)
include/Support/FileSystem.h (2)
45-45: LGTM! Version update aligns with LLVM 21.1.4 upgrade.The path update from
lib/clang/20tolib/clang/21correctly reflects the LLVM version bump.
42-42: All callers correctly handle the newstd::stringerror type—no updates needed.Verification confirms both call sites (bin/unit_tests.cc and bin/clice.cc) properly invoke
result.error()and pass the string to output functions. The breaking API change has been fully implemented.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests