chore: upgrade LLVM to 22.1.4#477
Conversation
Adapt codebase to LLVM 22 API changes: - ElaboratedType removed: delete all getAs<ElaboratedType>() calls - NestedNameSpecifier: pointer → value type, Kind::Identifier/NamespaceAlias removed - DependentTemplateSpecializationType merged into TemplateSpecializationType - TagDecl::getTypeForDecl() deleted → use ASTContext::getCanonicalTagType() - UsingType::getFoundDecl() → getDecl() - PredefinedSugarType (__size_t): desugar in hover to avoid internal names - InjectedClassNameType inherits TagType: USR encoding verified consistent - CompilerInstance::createDiagnostics VFS param removed - Options.inc moved to clang/Options/, OPTION macro gained 15th param - Driver::GetResourcesPath → clang::GetResourcesPath - fs::make_absolute → path::make_absolute - ClangTidyModuleRegistry.h → ClangTidyModule.h - Remove hover tag-keyword hack (LLVM 22 handles it natively) - Add clangOptions and clangTidyCustomModule to link deps
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR upgrades LLVM to 22.1.4, updates Clang library and option-table wiring, migrates several Clang API usages to LLVM 22 forms, rewrites dependent type and nested-name-specifier handling, and updates docs plus snapshots/tests to match the new behavior. ChangesLLVM 22 API Migration
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/semantic/find_target.cpp (1)
456-501: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore dependent template-specialization resolution.
TemplateSpecializationTypewith aDependentTemplateNamenow falls through this visitor, so names likeT::template rebind<U>can lose hover/go-to targets. Add a dependent-template-name branch before the alias/non-dependent handling and route it through the existing resolver path.🤖 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/find_target.cpp` around lines 456 - 501, Restore handling for dependent template specializations in VisitTemplateSpecializationType so dependent names do not fall through and lose targets. Add an explicit branch for type->getTemplateName().isDependent() before the alias/non-dependent logic, and route it through the existing resolver path used for dependent template names. Keep the current alias, template template parameter, CXXRecordDecl, and fallback cases unchanged for non-dependent specializations.
🧹 Nitpick comments (1)
tests/unit/index/usr_tests.cpp (1)
351-375: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove debug
llvm::errs()calls.These prints will run on every test execution and add noise; the
ASSERT_EQalready validates the invariant.♻️ Proposed cleanup
t2.run(); - llvm::errs() << "injected S: " << t1.lookup("f") << "\n"; - llvm::errs() << "explicit S<T>: " << t2.lookup("f") << "\n"; - ASSERT_EQ(t1.lookup("f"), t2.lookup("f")); }🤖 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 `@tests/unit/index/usr_tests.cpp` around lines 351 - 375, The InjectedClassName test in the USR tester currently emits debug output via llvm::errs() before the ASSERT_EQ, which adds noise to every test run. Remove the temporary stderr prints from the InjectedClassName case and keep only the assertion comparing t1.lookup("f") and t2.lookup("f"); use the test names and USRTester calls to locate the cleanup.
🤖 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.
Outside diff comments:
In `@src/semantic/find_target.cpp`:
- Around line 456-501: Restore handling for dependent template specializations
in VisitTemplateSpecializationType so dependent names do not fall through and
lose targets. Add an explicit branch for type->getTemplateName().isDependent()
before the alias/non-dependent logic, and route it through the existing resolver
path used for dependent template names. Keep the current alias, template
template parameter, CXXRecordDecl, and fallback cases unchanged for
non-dependent specializations.
---
Nitpick comments:
In `@tests/unit/index/usr_tests.cpp`:
- Around line 351-375: The InjectedClassName test in the USR tester currently
emits debug output via llvm::errs() before the ASSERT_EQ, which adds noise to
every test run. Remove the temporary stderr prints from the InjectedClassName
case and keep only the assertion comparing t1.lookup("f") and t2.lookup("f");
use the test names and USRTester calls to locate the cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29d41ac9-6751-4ae6-ba79-c9bd6556de52
📒 Files selected for processing (28)
cmake/llvm.cmakecmake/package.cmakedocs/en/changelog/llvm-changelog.mdsrc/command/argument_parser.cppsrc/command/argument_parser.hsrc/command/search_config.cppsrc/compile/compilation.cppsrc/compile/implement.hsrc/compile/tidy.cppsrc/feature/hover.cppsrc/index/usr_generation.cppsrc/semantic/ast_utility.cppsrc/semantic/filtered_ast_visitor.hsrc/semantic/find_target.cppsrc/semantic/resolver.cppsrc/semantic/resolver.hsrc/semantic/selection.cppsrc/semantic/semantic_visitor.hsrc/syntax/scan.cpptests/snapshots/hover/snapshot/auto.cpp.snap.ymltests/snapshots/hover/snapshot/docs.cpp.snap.ymltests/snapshots/hover/snapshot/lambdas.cpp.snap.ymltests/snapshots/hover/snapshot/tag_decls.cpp.snap.ymltests/snapshots/hover/snapshot/this_expr.cpp.snap.ymltests/snapshots/hover/snapshot/values.cpp.snap.ymltests/unit/feature/inlay_hint_tests.cpptests/unit/index/usr_tests.cpptests/unit/semantic/selection_tests.cpp
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
clice/src/semantic/find_target.cpp
Lines 498 to 500 in 39f8019
Restore dependent template specialization resolution
When the selected type is a dependent template specialization such as typename Traits<T>::template rebind<U>, getTemplateName().getAsTemplateDecl() is null; before this upgrade the dedicated dependent-template visitor queried HeuristicResolver::resolveTemplateSpecializationType. With LLVM 22 these nodes now arrive here as TemplateSpecializationType, but this fallback only handles non-dependent template decls, so go-to-definition/hover/reference targeting on those dependent template names returns no target.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
Retrigger CI after release |
- Remove debug llvm::errs() from InjectedClassName USR test - Fix stale comments: TransformDependentTemplateSPTType → TransformTemplateSpecializationType, remove ElaboratedType mention - Use default case in NNS kind switches for forward compatibility
There was a problem hiding this comment.
💡 Codex Review
clice/src/semantic/find_target.cpp
Line 456 in 9008031
Restore dependent template-specialization resolution
In LLVM 22 the old DependentTemplateSpecializationType cases now arrive here as TemplateSpecializationType, but this visitor never falls back to HeuristicResolver::resolveTemplateSpecializationType(const TemplateSpecializationType*). For a dependent template-id such as typename MetaFun::template apply<T>/typename A<T>::template rebind<U>, getAsTemplateDecl() and getAsCXXRecordDecl() are null, so explicit_reference_targets() returns no declaration and hover/go-to-definition lose the target that the removed visitor used to resolve.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| EXPECT_SIZE(2); | ||
| EXPECT_HINT("0", ": S1"); | ||
| EXPECT_HINT("1", ": S2::Inner<int>"); | ||
| EXPECT_HINT("1", ": Inner<int>"); |
There was a problem hiding this comment.
Accepting this expectation means class-member nested return types are now rendered without the owning class when only namespace qualifiers were meant to be suppressed. In this test auto y = bar(); is in namespace A::B, but Inner<int> is not a valid or unambiguous spelling there without S2::, so type inlay hints for nested types returned outside their parent class become misleading.
Useful? React with 👍 / 👎.
| bool TraverseTypeLoc(clang::TypeLoc X, bool traverse_qualifier = true) { | ||
| return traverse_node(&X, [&] { return Base::TraverseTypeLoc(X, traverse_qualifier); }); |
There was a problem hiding this comment.
When Clang calls this override with traverse_qualifier=false for declarations such as using enum ns::A, the selection tree no longer records the explicit NestedNameSpecifierLoc; the updated selection test now falls back from the ns:: qualifier to the whole UsingEnumDecl. That makes hover/go-to-definition on the ns token resolve the enum/using declaration rather than the namespace, so explicit qualifiers still need to be traversed or handled for this case.
Useful? React with 👍 / 👎.
1 participant
Summary
Upgrade LLVM dependency from 21.1.8 to 22.1.4. Single commit, pure API adaptation — no infrastructure changes (those landed in #476).
LLVM 22 Breaking Changes Adapted
All changes documented in
docs/en/changelog/llvm-changelog.mdwith upstream commit references.Type system (5 of 7 from the same mega-commit #147835):
ElaboratedTyperemoved — keyword/qualifier merged intoTagTypeInjectedClassNameTypenow inheritsTagTypeDependentTemplateSpecializationTypemerged intoTemplateSpecializationTypeTagDecl::getTypeForDecl()deleted →ASTContext::getCanonicalTagType()UsingType::getFoundDecl()→getDecl()PredefinedSugarType(__size_t) — desugar in hoverNNS: pointer → value type;
Kind::Identifier/NamespaceAliasremoved;NamespaceBaseDeclintroducedDriver/Frontend:
Options.incpath changed;OPTIONmacro 15th param;GetResourcesPathmoved;createDiagnosticsVFS param removedOther:
fs::make_absolute→path::make_absolute;ClangTidyModuleRegistry.hdeprecatedNotable behavior changes
struct/class/enumprefix (removed tag-keyword hack — LLVM 22 TypePrinter handles elaboration natively)PredefinedSugarTypedesugared in hover to avoid__size_tdisplayTest plan
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests