chore: upgrade LLVM to 22.1.8 - #586
Merged
Merged
Conversation
…ixes
Standardize all release artifact names to use proper target triples
(x86_64-unknown-linux-gnu, aarch64-apple-darwin, x86_64-pc-windows-msvc,
etc.) with version numbers in filenames: clice-{ver}.{triple}.{ext}.
Apply the three TODO(prebuilt-respin) fixes validated in ci-toolchain-test:
- Windows: switch to /MT static CRT (no MSVCP140.dll/VCRUNTIME140.dll)
- macOS: set explicit CMAKE_OSX_DEPLOYMENT_TARGET=15.0
- macOS: remove conda rpath hack (prebuilt now links system libc++)
Bump LLVM to 21.1.8+r3 for the respin.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR upgrades clice for LLVM 22. It updates Clang APIs, semantic type resolution, hover rendering, compiler setup, target-triple naming, packaging workflows, toolchain settings, documentation, and tests. ChangesLLVM 22 upgrade
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 8
🧹 Nitpick comments (2)
src/semantic/resolver.cpp (2)
517-533: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the inner
nameto avoid shadowing the parameter.Line 525 declares
nameinside a function whose parameter is alsoname. The inner value is the dependent template's identifier; the outer value is the member being looked up. Both are live in the same function. A distinct identifier removes the ambiguity.♻️ Proposed rename
- auto name = dependent->getName().getIdentifier(); - if(!name) { + auto* template_name = dependent->getName().getIdentifier(); + if(!template_name) { return {}; } - if(auto decl = preferred(lookup(dependent->getQualifier(), name))) { + if(auto decl = preferred(lookup(dependent->getQualifier(), template_name))) { TD = decl; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/semantic/resolver.cpp` around lines 517 - 533, Rename the inner `name` variable in the dependent-template resolution block to a distinct identifier, and update its use in the `preferred(lookup(...))` call; leave the function parameter `name` unchanged.
1987-2016: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the non-null precondition at line 1994.
resolve_dependent_templatedereferencesgetAsDependentTemplateName()without a check. The only caller guards it at line 819, so the code is correct today. An assert records the precondition for future callers.🛡️ Proposed guard
- auto& template_name = *TST->getTemplateName().getAsDependentTemplateName(); + auto* dependent = TST->getTemplateName().getAsDependentTemplateName(); + assert(dependent && "resolve_dependent_template requires a dependent template name"); + auto& template_name = *dependent;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/semantic/resolver.cpp` around lines 1987 - 2016, Add an assertion in resolve_dependent_template before dereferencing TST->getTemplateName().getAsDependentTemplateName(), documenting that the dependent template name must be non-null. Preserve the existing resolution flow and caller behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-llvm.yml:
- Around line 67-72: Update all aarch64 toolchain triple references in the
workflow matrix, cmake/toolchain.cmake cache key and path logic, and
toolchain_tests.cpp to consistently use aarch64-unknown-linux-gnu, preserving
the existing build and test behavior.
- Around line 204-218: Make the llvm_version workflow input default to 22.1.8,
or validate it before the build proceeds. Update the LLVM clone and
patch-selection logic to use llvm_version instead of hardcoded 21.1.8. Update
ARCHIVE so generated package names explicitly include the selected LLVM version
and cannot be confused with artifacts from another release.
In @.github/workflows/cross-test.yml:
- Around line 16-24: Add an explicit least-privilege permissions block for the
cross-test workflow, scoped at the workflow or job level around the linux-arm64
job, granting only contents: read. Do not add broader permissions unless an
existing workflow step demonstrably requires them.
In @.github/workflows/publish-clice.yml:
- Around line 47-48: Update the artifact validation step around LIVE to query
all six required matrix artifact names, excluding expired artifacts, and verify
that every expected package artifact is present. Fail the workflow before
release creation or asset uploads when any artifact is missing or expired, while
preserving promotion only when all six artifacts are available.
In @.github/workflows/publish-vscode.yml:
- Around line 142-157: Update the Determine vsix output name and Package
platform extension steps to keep RELEASE_TAG in the environment, construct
optional output arguments in a Bash array, and pass them using quoted array
expansion. Remove the shell-evaluated steps.vsix.outputs.out interpolation while
preserving the existing versioned filename and no-tag behavior.
In `@cmake/llvm.cmake`:
- Line 39: Update the archive naming logic around _FILENAME and the
CLICE_TARGET_STRING mapping in CMakeLists.txt to use one shared canonical triple
formatter. Ensure the x86_64 Linux GNU target consistently produces
x86_64-unknown-linux-gnu in both downloadable archive identifiers and embedded
metadata.
In `@cmake/package.cmake`:
- Line 4: Update the version argument in setup_llvm to request the published
LLVM 22.1.8 package instead of 21.1.8+r3, preserving the existing package
configuration structure.
In `@src/semantic/semantics.cpp`:
- Around line 487-490: Update TraverseQualifiedTypeLoc to name and forward its
traverse_qualifier parameter when calling TraverseTypeLoc, preserving the
caller’s false value instead of always re-enabling qualifier traversal.
---
Nitpick comments:
In `@src/semantic/resolver.cpp`:
- Around line 517-533: Rename the inner `name` variable in the
dependent-template resolution block to a distinct identifier, and update its use
in the `preferred(lookup(...))` call; leave the function parameter `name`
unchanged.
- Around line 1987-2016: Add an assertion in resolve_dependent_template before
dereferencing TST->getTemplateName().getAsDependentTemplateName(), documenting
that the dependent template name must be non-null. Preserve the existing
resolution flow and caller behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 25cd49da-cfb8-4c70-b4ee-6fa654c3998b
📒 Files selected for processing (36)
.claude/skills/upgrade-llvm/SKILL.md.github/workflows/build-llvm.yml.github/workflows/cross-pair.yml.github/workflows/cross-test.yml.github/workflows/native-test.yml.github/workflows/nightly.yml.github/workflows/publish-clice.yml.github/workflows/publish-vscode.yml.github/workflows/release-llvm.ymlcmake/llvm.cmakecmake/package.cmakecmake/toolchain.cmakedocs/en/changelog/llvm-changelog.mdscripts/build-llvm.pyscripts/check_artifact_deps.pyscripts/release-llvm.pysrc/command/argument_parser.cppsrc/command/argument_parser.hsrc/command/search_config.cppsrc/compile/compilation.cppsrc/feature/hover.cppsrc/index/usr_generation.cppsrc/semantic/display.cppsrc/semantic/resolver.cppsrc/semantic/resolver.hsrc/semantic/semantics.cppsrc/semantic/types.cppsrc/semantic/unifier.cppsrc/syntax/scan.cpptests/snap/hover/auto.snap.ymltests/snap/hover/docs.snap.ymltests/snap/hover/expressions.snap.ymltests/snap/hover/misc.snap.ymltests/snap/hover/tag_decls.snap.ymltests/snap/hover/this_expr.snap.ymltests/unit/semantic/selection_tests.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-llvm.yml:
- Line 159: Replace direct GitHub expression interpolation in the VERSION
assignments of both build steps with environment-variable passing, then read the
quoted shell variable inside Bash. Apply the same env-based pattern to
inputs.patch_ref in the patch step, ensuring untrusted workflow inputs cannot
alter the script syntax.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2340f90a-ea85-4de1-b5b7-2185317761a4
📒 Files selected for processing (2)
.github/workflows/build-llvm.ymlcmake/package.cmake
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Upgrades the prebuilt LLVM dependency from 21.1.8 to 22.1.8 and adapts clice to the LLVM 22 API. Highlights:
NestedNameSpecifieris a value type and types carry their elaborated keyword and qualifier themselves. Rewrote NNS handling in the template resolver, semantic visitor, unifier, display, and USR generation;rewrite_specifiercollapses to a single type-component rewrite since prefixes now live inside type nodes.TemplateSpecializationTypes with aDependentTemplateName. The resolver's DTST lookup/rewrite/pseudo-SFINAE paths merged into the TST paths.SUBCOMMANDIDS_OFFSETOPTION column ([llvm] Add subcommand support for OptTable llvm/llvm-project#155026): include-path and macro updates in the argument parser.getCanonicalTagType,getCanonicalTemplateSpecializationType,UsingType::getDecl,sys::path::make_absolute,clang::GetResourcesPath.Behavioral changes visible in features (all matching clang/clangd 22 rendering, pinned in snapshots):
__size_t (aka unsigned long)sugar,(unnamed enum)naming, namespace-qualified canonical class types in hover, and converted (qualified) template arguments in hover titles for implicit variable template specializations. One deliberate divergence from clangd: inlay type hints keep written class scopes (S2::Nested<int>) that LLVM 22'sSuppressScopewould now drop — restored by printing the outer node's written qualifier.Also cherry-picks the release-triple normalization + prebuilt-respin fixes (
/MT, macOS deployment target) that the 22.1.8 prebuilt was built with, and adds anLLVM 21 → 22section to the LLVM changelog documenting every breaking change with upstream references.Note: CI stays red until the pruned 22.1.8
clice-llvmrelease is published (release-llvm is running against this branch); the version pin incmake/package.cmakewill be bumped in a follow-up commit on this branch once it is up.Tests
All four suites pass locally against the 22.1.8 prebuilt: unit (1174), integration (341), smoke (3/3), snap (393). Snapshot updates are limited to the upstream rendering changes listed above; one selection-tree unit test was re-marked because dependent qualifier chains are now
DependentNameTypeLoccomponents with name-only ranges.