Add support for LLVM 21#807
Conversation
WalkthroughThis PR upgrades CI/tooling to Clang/LLVM 21, adds Clang‑21-aware CMake predicates and warning flags, introduces Clang‑21 suppression macros, applies targeted Clang‑21/static-analysis suppressions across sources and tests, makes one benchmark return type explicit, and updates .clang-tidy rules. Changes
Sequence Diagram(s)(omitted — changes are build/config/diagnostics/test edits, not a multi-component runtime flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
.circleci/config.yml(2 hunks).clang-tidy(1 hunks).github/workflows/build.yml(4 hunks).github/workflows/old-compilers.yml(1 hunks)CMakeLists.txt(5 hunks)art_internal_impl.hpp(2 hunks)benchmark/micro_benchmark_node_utils.hpp(1 hunks)qsbr.cpp(2 hunks)test/test_key_encode_decode.cpp(2 hunks)test/test_qsbr_ptr.cpp(1 hunks)test_heap.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qsbr.cpp (1)
art_internal_impl.hpp (1)
UNODB_DETAIL_RESTORE_CLANG_WARNINGS(1469-1469)
⏰ 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: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (23)
test/test_key_encode_decode.cpp (2)
681-682: LGTM – Pointer arithmetic suppression is appropriate here.The
std::copyoperation requires pointer arithmetic. Localized suppression in test code is the right approach.
706-707: LGTM – Pointer arithmetic suppression is appropriate here.The
std::memcpyoperation requires pointer arithmetic. Localized suppression in test code is the right approach.test_heap.cpp (1)
46-47: LGTM – Additional suppression is justified.Adding
misc-const-correctnessto the suppression list is appropriate sincemallocreturns a non-const pointer by design..circleci/config.yml (2)
10-10: Version bump to clang 21 looks correct.
62-62: Good improvement – using variable instead of hardcoded version.This makes future version updates easier and more maintainable.
test/test_qsbr_ptr.cpp (1)
314-315: LGTM – Pointer arithmetic suppression is justified.Computing the end iterator via pointer arithmetic is valid here, and the suppression is appropriately scoped.
qsbr.cpp (1)
234-283: LGTM – NRVO warning suppression is appropriate.The
-Wnrvowarning introduced in clang 21 can be overly pedantic. Scoping the suppression around this internal function is the right approach.art_internal_impl.hpp (1)
1447-1469: LGTM – NRVO warning suppression is appropriate for this private method.Consistent with the suppression pattern used in
qsbr.cppfor internal functions..github/workflows/old-compilers.yml (1)
472-516: LGTM – Clang 20 entries follow the established pattern.Since clang 21 is now the default, moving clang 20 to the old compilers workflow is the right approach. All 8 matrix entries (Release/Debug with and without sanitizers) are correctly structured.
.clang-tidy (1)
19-22: The review comment is based on incorrect assumptions.
cppcoreguidelines-pro-bounds-pointer-arithmeticis not present in the Checks line—it's not globally disabled at all. The check was either already removed before this PR or was never in the disabled list. Your review assumes it's still there and asks whether it should be removed, but there's nothing to remove. The PR objective has already been achieved.Likely an incorrect or invalid review comment.
.github/workflows/build.yml (7)
102-158: Job name updates are correct.All clang job names have been consistently updated from version 20 to 21 across Release, Debug, and sanitizer configurations.
296-298: LLVM repository and package installation updated correctly.The APT repository URL and clang-21 package names are properly configured.
302-302: Release build dependencies updated.The OpenMP, LLVM, and LLD packages are correctly versioned for clang 21.
308-308: Static analysis tools package updated.
314-314: clang-tidy package updated.
351-351: Compiler version variable updated.The V=21 assignment correctly drives all LLVM tool version selections.
404-404: scan-build path updated.The static analyzer path correctly references version 21.
benchmark/micro_benchmark_node_utils.hpp (1)
232-233: Explicit return type improves clarity.Replacing
autowithstd::vector<std::uint64_t>makes the function's contract explicit, which is better for a public API. The return type at line 242 matches this declaration.CMakeLists.txt (5)
410-410: Version check variable defined correctly.The
cxx_ge_21generator expression follows the established pattern and correctly checks for compiler version >= 21.0.
414-414: Platform condition correctly defined.The
is_clang_ge_21_not_windowsvariable properly combines compiler identity, version, and platform checks.
719-719: Warning flags correctly applied to targets.The generator expression properly gates clang 21 warnings on standalone builds with clang >= 21 on non-Windows platforms.
301-303: vptr sanitizer enabled for clang 21.Per the PR description,
-fsanitize=vptris now enabled when using UBSan with clang 21+. This will add runtime checks for virtual pointer correctness, which increases overhead but improves UB detection.
732-733: The review comment is incorrect. The code already handles this correctly.The
is_clang_genexvariable checks specifically forCXX_COMPILER_ID:Clang(line 394), which is Homebrew's clang. It's explicitly separate fromis_apple_clang_genex(line 395), which is Xcode's clang.Intel Mac CI jobs run with
macos-clang(AppleClang), so the condition on lines 732-733 will never trigger on them. Only machines actually using Homebrew's clang compiler will hit this path. No Intel Mac breakage exists—the conditional already excludes Xcode's clang.Likely an incorrect or invalid review comment.
laurynas-biveinis
force-pushed
the
llvm-21
branch
from
b686c47 to
ead6a15
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
.circleci/config.yml(2 hunks).clang-tidy(1 hunks).github/workflows/build.yml(4 hunks).github/workflows/old-compilers.yml(1 hunks)CMakeLists.txt(5 hunks)art_internal_impl.hpp(2 hunks)benchmark/micro_benchmark_node_utils.hpp(1 hunks)global.hpp(2 hunks)qsbr.cpp(2 hunks)test/test_key_encode_decode.cpp(2 hunks)test/test_qsbr_ptr.cpp(1 hunks)test_heap.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
global.hpp (1)
art_internal_impl.hpp (1)
UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS(1469-1469)
qsbr.cpp (1)
art_internal_impl.hpp (1)
UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS(1469-1469)
⏰ 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). (20)
- GitHub Check: build (clang 18 Release, Release, clang, 18)
- GitHub Check: build (clang 17 Debug, Debug, clang, 17)
- GitHub Check: build (clang 18 Release, Release, clang, 18)
- GitHub Check: build (clang 17 Debug, Debug, clang, 17)
- GitHub Check: build (clang 18 Release, Release, clang, 18)
- GitHub Check: build (clang 17 Debug, Debug, clang, 17)
- GitHub Check: build (clang 18 Release, Release, clang, 18)
- GitHub Check: build (clang 17 Debug, Debug, clang, 17)
- GitHub Check: build (clang 18 Release, Release, clang, 18)
- GitHub Check: build (clang 17 Debug, Debug, clang, 17)
- GitHub Check: build (clang 18 Release, Release, clang, 18)
- GitHub Check: build (clang 17 Debug, Debug, clang, 17)
- GitHub Check: build (clang 18 Release, Release, clang, 18)
- GitHub Check: build (clang 17 Debug, Debug, clang, 17)
- GitHub Check: build (clang 18 Release, Release, clang, 18)
- GitHub Check: build (clang 17 Debug, Debug, clang, 17)
- GitHub Check: build (clang 18 Release, Release, clang, 18)
- GitHub Check: build (clang 17 Debug, Debug, clang, 17)
- GitHub Check: build (clang 18 Release, Release, clang, 18)
- GitHub Check: build (clang 17 Debug, Debug, clang, 17)
🔇 Additional comments (11)
.github/workflows/old-compilers.yml (1)
472-517: LGTM—Clang 20 matrix entries are well‑integrated.The eight new Clang 20 matrix entries (Release/Debug across no sanitizers, ASan, TSan, UBSan) follow the established pattern consistently. Positioning between Clang 19 and GCC 10 is correct, and the existing workflow setup steps (line 756 LLVM 16+ repo check, line 808 MAINTAINER_MODE enablement for VERSION ≥ 13) apply correctly to Clang 20 without requiring additional configuration.
test_heap.cpp (1)
46-48: NOLINT extension is scoped correctlyAdding
misc-const-correctnesshere is consistent with the existing malloc-related suppressions and kept to a single call site. No functional issues.Please let clang-tidy/CI confirm this fully silences the intended warning and doesn’t mask anything else.
test/test_key_encode_decode.cpp (1)
89-90: Targeted suppressions look reasonableBoth added
NOLINTNEXTLINEdirectives are narrowly scoped to a single assertion / memcpy and match the intent of re‑enablingcppcoreguidelines-pro-bounds-pointer-arithmeticwhile keeping noise down. No code changes, just tooling alignment.Confirm via clang-tidy that there are no remaining unexpected hits from these checks in this file.
Also applies to: 707-708
test/test_qsbr_ptr.cpp (1)
313-316: Pointer-arithmetic suppression is fineSuppressing
cppcoreguidelines-pro-bounds-pointer-arithmeticfor&two_chars[1] + 1in this test is justified and tightly scoped. No behavioral change.Run clang-tidy on this file to ensure this is the only needed suppression after enabling the check.
.circleci/config.yml (1)
10-10: CircleCI clang 21 upgrade is coherentCLANG_VERSION=21, the LLVM APT repo uses
llvm-toolchain-noble-${CLANG_VERSION}, and all clang job names/uses are parameterized off that value. This keeps CircleCI aligned with the rest of the toolchain changes.Let the CircleCI clang builds run end‑to‑end to confirm the noble/21 repo and all
clang-${CLANG_VERSION}packages resolve correctly.Also applies to: 62-67, 102-155
benchmark/micro_benchmark_node_utils.hpp (1)
231-243: Explicit std::vector return type is fineMaking
generate_keys_to_limitreturnstd::vector<std::uint64_t>explicitly matches the actual return value and the existing call sites usingauto. No behavioral change, and this is friendlier to stricter clang diagnostics.Rebuild benchmarks with clang 21 to ensure this resolves whatever
auto-return warning you were targeting..github/workflows/build.yml (1)
102-159: GitHub Actions clang 21 migration is internally consistentJob names, APT repo (
llvm-toolchain-noble-21), installed packages (clang-21,clang-tools-21,clang-tidy-21,llvm-21,lld-21),V=21in the configure step, and/usr/bin/scan-build-21all line up. This should give a clean clang‑21 toolchain across both normal builds and static analysis.Let the Linux/clang jobs run; verify that all 21‑series packages install cleanly and that
scan-build-21andclang-tidy-21are actually present on the runner.Also applies to: 296-299, 302-303, 308-309, 314-315, 351-372, 404-405
CMakeLists.txt (1)
91-92: Clang‑21 CMake wiring (warnings, UBSan, macOS libc++) looks correct
CLANG_GE_21_CXX_WARNING_FLAGSis sensibly restricted viais_clang_ge_21_not_windows, so only standalone non‑Windows clang ≥21 sees the new warnings.cxx_ge_21/is_clang_ge_21_not_windowsfollow the existing version‑predicate pattern and won’t accidentally hit AppleClang.- Adding
-fsanitize=vptrunderSANITIZE_UBfor clang ≥21 is consistent with extending UBSan; linking still uses the generic-fsanitize=undefinedruntime, which is fine for vptr.- The extra Darwin link options for
${is_clang_genex}correctly target non‑Apple Clang on macOS and avoid touching AppleClang builds.No structural or correctness issues in this block.
Please run a clang‑21 + UBSan build on both Linux and Homebrew‑clang macOS to confirm:
- the new warnings actually fire where expected,
-fsanitize=vptrinstrumentation links cleanly, and- the Homebrew libc++ link flags resolve without duplicate‑library or ODR problems.
Also applies to: 301-303, 410-415, 719-723, 732-733
global.hpp (1)
356-362: LGTM! Clean implementation following established patterns.The version-gated macro definitions correctly delegate to the existing warning suppression infrastructure and are no-ops for older compilers. This mirrors the approach used for GCC version-specific warnings.
qsbr.cpp (1)
234-283: Suppression is appropriate for this function.The
register_thread()function has multiple return paths within loops and conditional blocks, which legitimately prevents Named Return Value Optimization. Suppressing the warning here is the right approach rather than attempting to restructure the QSBR thread registration logic.art_internal_impl.hpp (1)
1447-1469: Correct usage of the new suppression macros.The
leave_last_child()method has conditional logic and modifies state before returning, which can prevent NRVO. The warning suppression is correctly placed around the function definition.
laurynas-biveinis
force-pushed
the
llvm-21
branch
from
ead6a15 to
06854c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/test_key_encode_decode.cpp (1)
88-90: Remove or correctreadability/check— it's not a valid clang-tidy check name.
readability/checkuses a slash, which doesn't match any actual clang-tidy check. All other NOLINTs in the codebase (and all valid clang-tidy checks) use dashes:readability-simplify-boolean-expr,cppcoreguidelines-pro-bounds-pointer-arithmetic, etc. This NOLINT is a no-op and won't suppress anything. Either remove line 89 or replace it with the correct check name if you know what warning you're trying to suppress.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
.circleci/config.yml(3 hunks).clang-tidy(1 hunks).github/workflows/build.yml(4 hunks).github/workflows/old-compilers.yml(1 hunks)CMakeLists.txt(5 hunks)art_internal_impl.hpp(2 hunks)benchmark/micro_benchmark_node_utils.hpp(1 hunks)global.hpp(2 hunks)qsbr.cpp(2 hunks)test/test_key_encode_decode.cpp(2 hunks)test/test_qsbr_ptr.cpp(1 hunks)test_heap.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
global.hpp (1)
art_internal_impl.hpp (1)
UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS(1469-1469)
qsbr.cpp (1)
art_internal_impl.hpp (1)
UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS(1469-1469)
⏰ 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). (39)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (GCC 14 Release with ASan, ubuntu-latest, Release, gcc, ON)
- GitHub Check: build (GCC 14 Debug with ASan, ubuntu-latest, Debug, gcc, ON)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (GCC 14 Release with ASan, ubuntu-latest, Release, gcc, ON)
- GitHub Check: build (GCC 14 Release static analysis & cpplint, ubuntu-latest, Release, gcc, ON, ON)
- GitHub Check: build (GCC 14 Debug with ASan, ubuntu-latest, Debug, gcc, ON)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (GCC 14 Release with ASan, ubuntu-latest, Release, gcc, ON)
- GitHub Check: build (GCC 14 Release static analysis & cpplint, ubuntu-latest, Release, gcc, ON, ON)
- GitHub Check: build (GCC 14 Debug with ASan, ubuntu-latest, Debug, gcc, ON)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (GCC 14 Release with ASan, ubuntu-latest, Release, gcc, ON)
- GitHub Check: build (GCC 14 Release static analysis & cpplint, ubuntu-latest, Release, gcc, ON, ON)
- GitHub Check: build (GCC 14 Debug with ASan, ubuntu-latest, Debug, gcc, ON)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (GCC 14 Release with ASan, ubuntu-latest, Release, gcc, ON)
- GitHub Check: build (GCC 14 Release static analysis & cpplint, ubuntu-latest, Release, gcc, ON, ON)
- GitHub Check: build (GCC 14 Debug with ASan, ubuntu-latest, Debug, gcc, ON)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (GCC 14 Release with ASan, ubuntu-latest, Release, gcc, ON)
- GitHub Check: build (GCC 14 Release static analysis & cpplint, ubuntu-latest, Release, gcc, ON, ON)
- GitHub Check: build (GCC 14 Debug with ASan, ubuntu-latest, Debug, gcc, ON)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (GCC 14 Release with ASan, ubuntu-latest, Release, gcc, ON)
- GitHub Check: build (GCC 14 Release static analysis & cpplint, ubuntu-latest, Release, gcc, ON, ON)
- GitHub Check: build (GCC 14 Debug with ASan, ubuntu-latest, Debug, gcc, ON)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (GCC 14 Release with ASan, ubuntu-latest, Release, gcc, ON)
- GitHub Check: build (GCC 14 Release static analysis & cpplint, ubuntu-latest, Release, gcc, ON, ON)
- GitHub Check: build (GCC 14 Debug with ASan, ubuntu-latest, Debug, gcc, ON)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (GCC 14 Release with ASan, ubuntu-latest, Release, gcc, ON)
- GitHub Check: build (GCC 14 Release static analysis & cpplint, ubuntu-latest, Release, gcc, ON, ON)
- GitHub Check: build (GCC 14 Debug with ASan, ubuntu-latest, Debug, gcc, ON)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (GCC 14 Release with ASan, ubuntu-latest, Release, gcc, ON)
- GitHub Check: build (GCC 14 Release static analysis & cpplint, ubuntu-latest, Release, gcc, ON, ON)
- GitHub Check: build (GCC 14 Debug with ASan, ubuntu-latest, Debug, gcc, ON)
🔇 Additional comments (16)
test_heap.cpp (1)
44-48: Local clang-tidy suppression is fine hereExtending the NOLINT to include
misc-const-correctnesson thismalloccall keeps the warning noise local and doesn’t affect behavior. No issues.test/test_key_encode_decode.cpp (1)
681-683: Pointer‑arithmetic suppressions are narrowly scoped and safeBoth suppressed sites use pointer arithmetic (
kv.data() + sz/+ sz + 1) with sizes that are either:
- taken directly from
kv.size()(for the copy fromkey_view), or- validated via the preceding size/length expectations for the encoded key.
Given those invariants, the suppressions of
cppcoreguidelines-pro-bounds-pointer-arithmeticare justified and keep the check enabled globally without risking UB here.Also applies to: 706-708
.circleci/config.yml (1)
10-11: Clang‑21 wiring and LLVM repo usage look internally consistentDefaulting
CLANG_VERSIONto 21, switching the apt source tollvm-toolchain-noble-${CLANG_VERSION}, and installingclang-${CLANG_VERSION}/clang-tidy-${CLANG_VERSION}/llvm-${CLANG_VERSION}/lld-${CLANG_VERSION}are all aligned. Using unversionedlibomp5is reasonable on Ubuntu 24.04; CI will catch any package‑name drift.Also applies to: 62-67, 76-77
.github/workflows/old-compilers.yml (1)
472-517: clang‑20 matrix entries match the existing patternThe new clang‑20 Debug/Release (+sanitizer) jobs are consistent with the clang 16–19 entries and integrate cleanly with the existing
VERSION‑driven LLVM setup logic in this workflow.global.hpp (1)
95-102: clang‑21 warning macros are correctly version‑gatedThe new
UNODB_DETAIL_DISABLE_CLANG_21_WARNING/UNODB_DETAIL_RESTORE_CLANG_21_WARNINGSmacros are documented and implemented in a sane way: on__clang_major__ >= 21they reduce to the existing generic disable/restore, and they’re harmless no‑ops elsewhere, so uses around new warnings (like-Wnrvo) won’t break older toolchains.Also applies to: 356-362
art_internal_impl.hpp (1)
1447-1469: NRVO suppression is scoped correctly; just confirm the warning nameWrapping
leave_last_childwithUNODB_DETAIL_DISABLE_CLANG_21_WARNING("-Wnrvo")/...RESTORE...cleanly scopes the suppression and doesn’t affect logic. Assuming clang 21 actually uses-Wnrvoas the diagnostic flag, this is fine; if the spelling differs you’ll just get an “unknown warning option” noise, so double‑check against the toolchain you’re targeting.benchmark/micro_benchmark_node_utils.hpp (1)
231-243: Explicitstd::vector<std::uint64_t>return type is a clean improvementSpelling out the return type for
generate_keys_to_limitmatches how it’s actually used, improves readability, and doesn’t change behavior or any call site expectations..clang-tidy (1)
19-22: clang‑tidy config matches the stated intentYou’ve:
- Re‑enabled
cppcoreguidelines-pro-bounds-pointer-arithmeticglobally and moved to local suppressions, and- Globally disabled
modernize-use-scoped-lockandllvm-prefer-static-over-anonymous-namespacewith clear rationale comments.The
Checks:line matches the comments and check spellings; configuration is coherent..github/workflows/build.yml (1)
102-404: LGTM! Clang 21 toolchain upgrade is consistent.All references to Clang 20 have been systematically updated to Clang 21 across job names, repository URLs, package names, and tool paths. The changes are uniform and complete.
CMakeLists.txt (5)
91-92: LGTM! New Clang 21 warning flags defined correctly.The warning flags list is clean and follows the existing pattern for version-specific warnings.
301-303: LGTM! vptr sanitizer addition enhances UB detection.Enabling
-fsanitize=vptrfor Clang 21+ appropriately extends undefined behavior detection to catch virtual pointer corruption. The version guard ensures compatibility.
410-410: LGTM! Version predicate follows established pattern.The
cxx_ge_21predicate is consistent with existing version checks.
414-414: LGTM! Compound predicate correctly combines version and platform checks.The
is_clang_ge_21_not_windowspredicate follows the pattern used for other version-platform combinations.
719-719: LGTM! Warning flags applied consistently.The conditional application of Clang 21 warning flags follows the established pattern for version-specific warnings.
test/test_qsbr_ptr.cpp (1)
314-315: LGTM! More specific linter suppression.Refining the suppression to explicitly name
cppcoreguidelines-pro-bounds-pointer-arithmeticis better practice than a generic comment. The pointer arithmetic is intentional for testing iterator behavior.qsbr.cpp (1)
234-283: LGTM! Scoped warning suppression for Clang 21 compatibility.The
-Wnrvosuppression aroundregister_thread()is narrowly scoped and uses version-gated macros. The function's complex control flow with multiple return points likely triggers the warning, making suppression appropriate.
laurynas-biveinis
force-pushed
the
llvm-21
branch
2 times, most recently
from
1571fe7 to
2325ca1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
.circleci/config.yml(4 hunks).clang-tidy(1 hunks).github/workflows/build.yml(4 hunks).github/workflows/old-compilers.yml(1 hunks)CMakeLists.txt(6 hunks)art_internal_impl.hpp(2 hunks)benchmark/micro_benchmark_node_utils.hpp(1 hunks)global.hpp(2 hunks)qsbr.cpp(2 hunks)test/test_key_encode_decode.cpp(2 hunks)test/test_qsbr_ptr.cpp(1 hunks)test_heap.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
global.hpp (1)
art_internal_impl.hpp (1)
UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS(1469-1469)
qsbr.cpp (1)
art_internal_impl.hpp (1)
UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS(1469-1469)
⏰ 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: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (18)
test/test_qsbr_ptr.cpp (1)
314-315: LGTM!The inline suppression is appropriate. Computing one‑past‑end via pointer arithmetic is valid C++ and necessary here to avoid triggering libstdc++ debug assertions (as the comment explains). This aligns with the PR's approach of re‑enabling the check globally and suppressing it inline only where needed.
test/test_key_encode_decode.cpp (2)
681-682: LGTM!The suppression is appropriate. Pointer arithmetic
kv.data() + szis standard practice for defining ranges and is used correctly here withstd::copy.
706-707: LGTM!The suppression is justified. Pointer arithmetic
kv.data() + sz + 1is valid and necessary for accessing subsequent bytes in the key view buffer..github/workflows/old-compilers.yml (1)
472-517: LGTM!The clang 20 matrix entries are correctly structured and consistent with the existing clang 19 variants. Adding clang 20 to the old-compilers workflow is appropriate as clang 21 becomes the current version.
.clang-tidy (1)
19-22: LGTM!The changes align with the PR's strategy: re‑enabling
cppcoreguidelines-pro-bounds-pointer-arithmeticglobally and disabling two new checks (modernize-use-scoped-lockandllvm-prefer-static-over-anonymous-namespace) with clear rationale..circleci/config.yml (3)
10-10: LGTM!The CLANG_VERSION bump to 21 aligns with the PR's objective.
62-62: LGTM!Parameterizing the apt source with
${CLANG_VERSION}makes version updates cleaner.
155-155: Why remove parallel builds?The change from
make -j2 -ktomake -kremoves parallelism. This could significantly increase build time. Was this intentional? If so, what's the reason? If not, restore the-j2flag.benchmark/micro_benchmark_node_utils.hpp (1)
232-233: LGTM!Explicitly spelling out the return type
std::vector<std::uint64_t>improves clarity without changing behavior. This aligns with the PR's objective to replace some auto return types.global.hpp (1)
95-102: LGTM!The new clang 21 warning suppression macros follow the established pattern for versioned compiler checks and are properly documented. They enable scoped suppression of clang 21–specific warnings (like
-Wnrvo) in the codebase.Also applies to: 356-362
test_heap.cpp (1)
46-47: LGTM!The addition of
misc-const-correctnessto the NOLINTNEXTLINE suppression is appropriate. Themalloccall in this custom allocator cannot satisfy const-correctness requirements in the way clang 21 expects.qsbr.cpp (1)
234-283: LGTM—warning suppression properly scoped.The Clang 21 NRVO warning suppression is limited to this function only. Given the multiple return paths and complex control flow, this is reasonable.
.github/workflows/build.yml (1)
102-154: LGTM—consistent version bump to Clang 21.All references updated from Clang 20 to 21 across job names, package dependencies, and tool paths. No issues.
Also applies to: 296-314, 351-351, 404-404
CMakeLists.txt (4)
91-92: LGTM—warning flags properly defined.The Clang 21 warning flags are correctly defined without the trailing empty string that was flagged in previous reviews.
301-303: LGTM—vptr sanitizer enabled for Clang 21.Enabling
-fsanitize=vptrfor Clang 21+ under UBSan is a good improvement for catching virtual pointer undefined behavior. Properly scoped to the right condition.
686-686: LGTM—reasonable workaround for IWYU.Adding
-Wno-unknown-warning-optionprevents IWYU from failing on warning flags it doesn't recognize. This is a pragmatic solution.
734-737: Paths are correct, but predicates are wrong.The Intel (/usr/local) and ARM64 (/opt/homebrew) paths are correct. However, these lines depend on the
is_darwin_clang_x86_64andis_darwin_clang_arm64predicates, which have the wrong condition (see comment on lines 404-405).art_internal_impl.hpp (1)
1447-1469: LGTM—warning suppression properly scoped.The Clang 21 NRVO warning suppression is limited to this internal method. The scope is appropriate and matches the pattern used in qsbr.cpp.
laurynas-biveinis
force-pushed
the
llvm-21
branch
from
2325ca1 to
30914ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
.github/workflows/build.yml (1)
291-303: Align libomp runtime with LLVM 21; other clang‑21 wiring looks goodMost of the clang‑21 wiring here is coherent: the noble‑21 repo,
clang-21,clang-tools-21,clang-tidy-21,V=21in CMake, andscan-build-21all line up.The one thing to double‑check is:
sudo apt-get install -y libomp5 llvm-21 lld-21— using unversionedlibomp5may again pull the OpenMP runtime from Ubuntu’s default repos instead of the llvm‑toolchain‑21 repo. That can leave you with clang 21 plus an older libomp. Prefer the versioned runtime (libomp5-21) if that package still exists for noble, or otherwise confirm thatlibomp5resolves to the intended LLVM 21 runtime onubuntu-latest.#!/bin/bash set -euo pipefail # Show which libomp* packages are available/installed on GitHub's ubuntu-latest apt-cache policy libomp5 libomp5-21 || true # Cross-check llvm-toolchain-noble-21 package names for libomp curl -s "http://apt.llvm.org/noble/dists/llvm-toolchain-noble-21/main/binary-amd64/Packages.gz" \ | gunzip \ | grep -A5 -E '^Package: libomp'Also applies to: 307-315, 351-352, 404-405
.circleci/config.yml (1)
10-10: Re‑check libomp package choice; dropping-j2slows buildsTwo points here:
- Using unversioned
libomp5alongsidellvm-${CLANG_VERSION}/lld-${CLANG_VERSION}can pull an OpenMP runtime from the distro repos rather than the llvm-toolchain-${CLANG_VERSION} repo, risking version skew with Clang 21. Please confirm whatlibomp5resolves to on CircleCI’s Ubuntu 24.04 +llvm-toolchain-noble-${CLANG_VERSION}; if it’s not the LLVM 21 runtime, switch back to the versioned package (e.g.libomp5-21) to keep the toolchain consistent.- Changing
make -j2 -ktomake -khalves parallelism on thearm.mediumexecutor. If this wasn’t done to avoid flakiness/OOM, consider restoring a-jflag to keep CI times under control.#!/bin/bash set -euo pipefail # Inspect available libomp packages in the llvm-toolchain-noble-21 repo for arm64 curl -s "http://apt.llvm.org/noble/dists/llvm-toolchain-noble-21/main/binary-arm64/Packages.gz" \ | gunzip \ | grep -A5 -E '^Package: libomp' echo echo "Now check what apt would install for libomp5:" apt-cache policy libomp5 || trueAlso applies to: 62-63, 76-78, 155-155
CMakeLists.txt (2)
404-405: Still using wrong predicate—not scoped to Clang 21.Past reviews flagged that these predicates use
${is_clang_genex}(any Clang version) instead of${is_clang_ge_21_not_windows}(Clang 21+). The issue was marked as addressed in commit 1571fe7, but the code still uses${is_clang_genex}. This contradicts the PR objective: "On macOS with Homebrew-installed LLVM 21, link against Homebrew-packaged libc++."Replace
${is_clang_genex}with${is_clang_ge_21_not_windows}in both predicates.
734-737: Linker flags apply to all Clang versions due to wrong predicates.These linker flags use
is_darwin_clang_x86_64andis_darwin_clang_arm64, which themselves use the wrong predicate (is_clang_genexinstead ofis_clang_ge_21_not_windows). This means the Homebrew libc++ linker flags will apply to ALL Clang versions on Darwin, not just Clang 21 as intended by the PR objective.Fix the predicates at lines 404-405 first, then these flags will be correctly scoped.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
.circleci/config.yml(4 hunks).clang-tidy(1 hunks).github/workflows/build.yml(4 hunks).github/workflows/old-compilers.yml(1 hunks)CMakeLists.txt(6 hunks)art_internal_impl.hpp(2 hunks)benchmark/micro_benchmark_node_utils.hpp(1 hunks)fuzz_deepstate/test_art_fuzz_deepstate.cpp(1 hunks)fuzz_deepstate/test_qsbr_fuzz_deepstate.cpp(1 hunks)global.hpp(2 hunks)qsbr.cpp(2 hunks)test/test_key_encode_decode.cpp(2 hunks)test/test_qsbr_ptr.cpp(1 hunks)test_heap.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
global.hpp (1)
art_internal_impl.hpp (1)
UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS(1469-1469)
qsbr.cpp (1)
art_internal_impl.hpp (1)
UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS(1469-1469)
🔇 Additional comments (16)
.github/workflows/old-compilers.yml (1)
472-517: Well-structured addition of Clang 20 test matrix.The new clang-20 entries are properly positioned after clang-19 and follow the established pattern exactly: 8 variants covering both Release/Debug builds with all sanitizer combinations (ASan, TSan, UBSan). YAML syntax and indentation are correct. This aligns with the PR objective to add Clang 20 to the old-compilers workflow.
benchmark/micro_benchmark_node_utils.hpp (1)
232-233: LGTM. Explicit return type is clearer.The change from
autotostd::vector<std::uint64_t>makes the interface explicit without altering behavior. Good move for LLVM 21 compatibility.test/test_qsbr_ptr.cpp (1)
311-316: Scoped suppression for pointer arithmetic is appropriateThe NOLINT is narrowly scoped to the
cendpointer arithmetic and aligns with re-enablingcppcoreguidelines-pro-bounds-pointer-arithmetic; no behavioral impact.test_heap.cpp (1)
43-55: Additional NOLINT check is fineExtending the suppression list on the malloc call is consistent with the existing pattern and remains tightly scoped to this operator new.
fuzz_deepstate/test_art_fuzz_deepstate.cpp (1)
234-287: Reasonable suppression of DeepState array-bound false positiveThe NOLINT is tightly scoped to the
deepstate::OneOfcall inside the fuzz loop and does not change semantics; acceptable for silencing analyzer noise in test code..clang-tidy (1)
19-22: Clang‑tidy configuration matches stated policyThe comments and
Checksline now:
- re-enable
cppcoreguidelines-pro-bounds-pointer-arithmetic, and- globally disable
modernize-use-scoped-lockandllvm-prefer-static-over-anonymous-namespace,which is consistent with the documented rationale.
qsbr.cpp (1)
234-283: Clang‑21 NRVO suppression is correctly scopedWrapping
qsbr::register_threadwithUNODB_DETAIL_DISABLE_CLANG_21_WARNING("-Wnrvo")/UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS()localizes the NRVO workaround to this function and is a no-op on other compilers/Clang versions; no behavior change..github/workflows/build.yml (1)
102-159: Clang 21 matrix expansion looks consistentThe clang job matrix has been cleanly bumped from 20→21 with matching combinations for build type and sanitizers plus separate static-analysis jobs; no gaps or asymmetries spotted.
global.hpp (1)
95-102: Clang‑21 warning macros are correctly guarded and integratedThe new
UNODB_DETAIL_DISABLE_CLANG_21_WARNING/UNODB_DETAIL_RESTORE_CLANG_21_WARNINGSdocs and definitions:
- mirror the existing clang/GCC versioned warning helpers,
- are only active for
__clang__ && __clang_major__ >= 21, and- degrade to no‑ops elsewhere,
which is exactly what you want for selectively suppressing 21‑only diagnostics like
-Wnrvo.Also applies to: 356-362
art_internal_impl.hpp (1)
1447-1469: LGTM.The NRVO warning suppression is properly scoped to the
leave_last_childfunction and uses the correct project-wide macros for Clang 21 compatibility.CMakeLists.txt (6)
91-92: LGTM.The Clang 21 warning flags are properly defined with no trailing empty strings.
301-303: LGTM.The vptr sanitizer is correctly enabled only for Clang 21+ with UBSan, matching the PR objectives.
412-412: LGTM.The Clang 21+ version predicate is correctly defined.
416-416: LGTM.The Clang 21+ non-Windows predicate is correctly defined.
686-686: LGTM.Adding
-Wno-unknown-warning-optionprevents IWYU from failing on Clang 21 warning flags it doesn't recognize.
721-721: LGTM.The Clang 21 warning flags are correctly applied only for Clang 21+ on non-Windows platforms.
laurynas-biveinis
force-pushed
the
llvm-21
branch
from
30914ca to
f781f64
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #807 +/- ##
==========================================
- Coverage 98.33% 98.30% -0.04%
==========================================
Files 35 35
Lines 6493 6493
==========================================
- Hits 6385 6383 -2
- Misses 108 110 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
laurynas-biveinis
force-pushed
the
llvm-21
branch
from
f781f64 to
0aea855
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
fuzz_deepstate/test_qsbr_fuzz_deepstate.cpp (1)
614-762: Add explanation for the array bounds false positive.The suppression wraps the entire 147-line test function without explaining why the array bounds warning is a false positive. If specific operations trigger the warning, narrow the suppression scope and document why it's safe.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
.clang-tidy(1 hunks).github/workflows/build.yml(5 hunks).github/workflows/old-compilers.yml(1 hunks)CMakeLists.txt(6 hunks)art_internal_impl.hpp(2 hunks)benchmark/micro_benchmark_node_utils.hpp(1 hunks)fuzz_deepstate/test_art_fuzz_deepstate.cpp(2 hunks)fuzz_deepstate/test_qsbr_fuzz_deepstate.cpp(2 hunks)global.hpp(2 hunks)qsbr.cpp(2 hunks)test/test_key_encode_decode.cpp(2 hunks)test/test_qsbr_ptr.cpp(1 hunks)test_heap.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
global.hpp (1)
art_internal_impl.hpp (1)
UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS(1469-1469)
⏰ 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: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (23)
.github/workflows/old-compilers.yml (1)
472-517: Clang 20 matrix additions look good.The eight new clang 20 entries are correctly structured, follow the established pattern from clang 19, and are properly positioned. YAML syntax and field values are correct.
test_heap.cpp (1)
45-48: Lint suppression extension is fineExtending the NOLINT list on the malloc call with
misc-const-correctnessis consistent with this test-only allocator shim and does not affect behavior.test/test_key_encode_decode.cpp (2)
676-683: Switch tostd::copy_nis correct and clearerUsing
std::copy_n(kv.data(), sz, a.begin())copies exactlyszbytes into the backing vector without pointer arithmetic and matches the vector size semantics.
703-707:memcpysource change preserves correctness and removes pointer arithmeticReading the pad length via
std::memcpy(&tmp, &kv[sz + 1], sizeof(st))is within the established key_view bounds and avoids raw-pointer arithmetic onkv.data().fuzz_deepstate/test_art_fuzz_deepstate.cpp (1)
204-312: Scoped clang-analyzer suppression around the fuzz test is acceptableWrapping
DeepStateFuzzwithNOLINTBEGIN/END(clang-analyzer-security.ArrayBound)cleanly localizes the analyzer suppression to this test without touching runtime behavior..clang-tidy (1)
19-22: Clang‑tidy configuration matches the documented intentComments and
Checks:now align:modernize-use-scoped-lockandllvm-prefer-static-over-anonymous-namespaceare explicitly disabled, whilecppcoreguidelines-pro-bounds-pointer-arithmeticis no longer globally suppressed.art_internal_impl.hpp (1)
1447-1469: NRVO warning suppression is scoped correctlyUsing
UNODB_DETAIL_DISABLE_CLANG_21_WARNING("-Wnrvo")/UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS()aroundleave_last_childcleanly contains the Clang‑21 NRVO warning without touching the function’s behavior.benchmark/micro_benchmark_node_utils.hpp (1)
231-243: Explicitstd::vector<std::uint64_t>return type is an improvementSpelling out the return type for
generate_keys_to_limitmakes the API clearer without changing behavior; existing uses remain valid.qsbr.cpp (1)
234-283: QSBRregister_threadNRVO suppression is safe and localizedWrapping
qsbr::register_thread()with the clang‑21 NRVO suppression macros leaves the epoch/threads state machine untouched while isolating the toolchain quirk.global.hpp (2)
95-102: New clang‑21 warning macro docs are clear and accurateThe added documentation for
UNODB_DETAIL_DISABLE_CLANG_21_WARNING/UNODB_DETAIL_RESTORE_CLANG_21_WARNINGScorrectly describes their purpose and pairing.
356-362: Clang‑21‑specific warning macros are correctly gatedConditionally mapping
UNODB_DETAIL_DISABLE_CLANG_21_WARNING/UNODB_DETAIL_RESTORE_CLANG_21_WARNINGSto the generic disable/restore machinery only when__clang_major__ >= 21keeps older compilers and GCC/MSVC unaffected while enabling targeted suppressions on clang‑21.test/test_qsbr_ptr.cpp (1)
314-315: Inline suppression correctly applied.The suppression is appropriate for the intentional pointer arithmetic needed to avoid libstdc++ debug assertion issues.
.github/workflows/build.yml (6)
102-158: LGTM on Linux Intel clang upgrade.Job names and configurations consistently updated from clang 20 to clang 21.
316-354: LGTM on Linux ARM64 clang upgrade.Job names consistently updated from clang 20 to clang 21.
392-394: LGTM on APT repository and clang package updates.Repository and main clang package correctly updated to version 21.
404-410: LGTM on static analysis tools updates.Tools consistently updated from version 20 to version 21.
447-467: LGTM on clang compiler version variable.Version correctly set to 21 for clang compiler configuration.
500-501: LGTM on scan-build update.Static analysis tool correctly updated to version 21.
CMakeLists.txt (5)
91-92: LGTM on new clang 21 warning flags.Flags are properly defined for clang 21+.
410-416: LGTM on clang 21 predicates.Version checks and platform-specific predicates correctly defined for clang 21 support.
721-721: LGTM on warning flags application.Clang 21 warning flags properly applied via generator expression.
734-737: LGTM on Darwin library paths.Architecture-specific Homebrew LLVM library paths correctly configured for both x86_64 and ARM64 using the proper predicates.
301-303: Review comment is incorrect.The condition at line 301 is
if(is_clang AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 21.0), whereis_clangmatches only LLVM Clang (compiler ID == "Clang"), not Apple Clang. Apple Clang (compiler ID == "AppleClang") is explicitly excluded. The code already properly restricts-fsanitize=vptrto LLVM Clang >= 21.0 and does not enable it for Apple Clang.Likely an incorrect or invalid review comment.
laurynas-biveinis
force-pushed
the
llvm-21
branch
from
0aea855 to
42f61a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/build.yml (1)
398-398: Past review not fully reflected in current code.The past review indicated
libomp5should be changed tolibomp5-21for version consistency withllvm-21andlld-21, and was marked as addressed. However, the current code still shows the unversionedlibomp5.If the unversioned package is intentional (perhaps it's an alias or metapackage), add a comment explaining why. Otherwise, this should be
libomp5-21.Run this script to check if the unversioned package is a known alias or if the versioned package exists:
#!/bin/bash # Description: Verify libomp5 package naming in LLVM 21 repository # Check what libomp packages are available in LLVM 21 repo echo "Checking LLVM 21 repository package listing..." curl -s "http://apt.llvm.org/noble/dists/llvm-toolchain-noble-21/main/binary-amd64/Packages" | grep -A10 "^Package: libomp" || echo "Could not fetch package list"CMakeLists.txt (1)
690-690: Add documentation for the IWYU workaround.The
-Wno-unknown-warning-optionflag suppresses IWYU errors for Clang-21-specific warnings. Add a brief comment above this line explaining:
- Why the flag is needed (IWYU doesn't recognize Clang 21 warning flags)
- When it can be removed (once IWYU gains Clang 21 support)
Example:
+ # IWYU doesn't recognize Clang 21 warning flags yet; suppress unknown-warning errors + # TODO: Remove once IWYU supports Clang 21 (track at IWYU issue tracker) set(DO_IWYU "${IWYU_EXE};-Wno-unknown-warning-option")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
.clang-tidy(1 hunks).github/workflows/build.yml(5 hunks).github/workflows/old-compilers.yml(1 hunks)CMakeLists.txt(6 hunks)art_internal_impl.hpp(2 hunks)benchmark/micro_benchmark_node_utils.hpp(1 hunks)fuzz_deepstate/test_art_fuzz_deepstate.cpp(2 hunks)fuzz_deepstate/test_qsbr_fuzz_deepstate.cpp(2 hunks)global.hpp(2 hunks)qsbr.cpp(2 hunks)test/test_key_encode_decode.cpp(2 hunks)test/test_qsbr_ptr.cpp(1 hunks)test_heap.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
global.hpp (1)
art_internal_impl.hpp (1)
UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS(1469-1469)
test/test_key_encode_decode.cpp (1)
portability_builtins.hpp (1)
bswap(25-25)
⏰ 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). (12)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 21 Debug with TSan (Linux ARM64), ubuntu-24.04-arm, Debug, clang, ON)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 21 Debug with TSan (Linux ARM64), ubuntu-24.04-arm, Debug, clang, ON)
🔇 Additional comments (21)
test_heap.cpp (1)
45-47: Extended NOLINT list is acceptable for this debug-only operator new.The extra suppression is tightly scoped to this intentionally non-idiomatic
mallocuse and doesn’t hide any real issue here.test/test_qsbr_ptr.cpp (1)
311-316: Pointer-arithmetic suppression in Cend test is reasonable.You’re intentionally forming a one-past-the-end pointer for comparison; adding a focused suppression instead of contorting the test further is fine.
test/test_key_encode_decode.cpp (1)
677-683: Pointer-arithmetic cleanup inkey_factoryanddo_simple_pad_testis correct.
std::copy_n(kv.data(), sz, a.begin())andstd::memcpy(&tmp, &kv[sz + 1], sizeof(st))both respect the established size invariants and keep the operations within bounds while avoiding the previous raw pointer math.Also applies to: 703-707
fuzz_deepstate/test_qsbr_fuzz_deepstate.cpp (1)
612-615: Scoped ArrayBound suppression forQSBR DeepStateFuzzis consistent and acceptable.The begin/end suppression cleanly wraps just the fuzz test and aligns with the pattern used in the ART fuzz; no behavior is affected.
Also applies to: 760-762
.clang-tidy (1)
19-22: .clang-tidy comment and disabled-check list are now aligned.The explanatory bullets match the Checks string, and re-enabling the pointer-arithmetic rule while explicitly disabling the two listed checks is coherent with the stated tooling strategy.
.github/workflows/old-compilers.yml (1)
472-517: clang-20 matrix entries correctly mirror the clang-19 combinations.The new Release/Debug + sanitizer variants are consistent with the existing clang blocks and integrate with the same VERSION-driven setup logic; nothing stands out structurally. Make sure CI actually provisions clang-20 and that this job runs green with those entries enabled.
art_internal_impl.hpp (1)
1447-1470: Clang-21 warning suppression aroundleave_last_childis well-scoped.The disable/restore macros cleanly wrap just this function, and the RAII + return logic is unchanged, so you get the diagnostic quieting without touching behavior.
qsbr.cpp (1)
234-283: LGTM: Warning suppression correctly scoped.The Clang-21 NRVO warning suppression is properly applied around the
register_threadfunction without altering its logic or signature.benchmark/micro_benchmark_node_utils.hpp (1)
232-233: LGTM: Explicit return type improves API clarity.Replacing
autowithstd::vector<std::uint64_t>makes the function signature self-documenting without altering behavior.global.hpp (2)
95-102: LGTM: Documentation follows established conventions.The documentation for the Clang-21 warning macros is clear and consistent with the existing warning macro patterns in this file.
356-362: LGTM: Macro implementation follows established pattern.The Clang-21 warning suppression macros correctly check the compiler version and delegate to the generic warning macros, with proper no-op fallback for other compilers.
.github/workflows/build.yml (5)
102-158: LGTM: Workflow names updated consistently.All Linux Intel Clang workflow entries correctly updated from version 20 to 21.
316-354: LGTM: ARM64 workflow names updated consistently.All Linux ARM64 Clang workflow entries correctly updated from version 20 to 21.
392-394: LGTM: Repository and Clang package updated correctly.The apt repository and clang package references correctly updated to version 21.
404-410: LGTM: Static analysis tools updated to version 21.Both
clang-tools-21andclang-tidy-21correctly updated.
447-500: LGTM: Build configuration and scan-build updated.The compiler version variable and scan-build invocation correctly updated to version 21.
CMakeLists.txt (5)
91-92: LGTM: Warning flags defined correctly.The Clang 21 warning flags are properly defined without the trailing empty string that was flagged in a past review.
301-310: LGTM: vptr sanitizer correctly enabled for Clang 21+.The
-fsanitize=vptrflag is properly added to both compiler and linker flags specifically for Clang 21 and later, aligning with the PR objective.
414-420: LGTM: Predicates correctly defined for Clang 21.The new generator expression predicates properly check for Clang 21+ and correctly scope Darwin-specific conditions to the appropriate compiler version, addressing past review concerns.
725-725: LGTM: Clang 21 warning flags applied correctly.The new warning flags are properly gated by standalone build and Clang 21+ version checks, consistent with existing warning flag patterns.
738-741: LGTM: Darwin linker paths correctly configured for Clang 21.The architecture-specific Homebrew libc++ paths are properly scoped to Clang 21+ on macOS, with correct paths for both Intel (/usr/local) and Apple Silicon (/opt/homebrew), addressing past review concerns.
laurynas-biveinis
force-pushed
the
llvm-21
branch
from
42f61a1 to
5d1701c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
fuzz_deepstate/test_art_fuzz_deepstate.cpp (1)
204-205: Consider documenting why theArrayBoundsuppression is safe.The NOLINT block is well-scoped to this fuzz test, but adding a short comment explaining the bounds invariant (why the analyzer is wrong here) would make the suppression easier to trust later.
Also applies to: 311-312
.github/workflows/build.yml (1)
398-402: Uselibomp5-21instead of unversionedlibomp5to match LLVM 21.With
llvm-toolchain-noble-21andllvm-21/lld-21, pulling the unversionedlibomp5risks binding to an older OpenMP runtime. You should installlibomp5-21here for version consistency with the rest of the toolchain, as previously suggested.Confirm via
apt-cache search libomp5-21(or by running this workflow) thatlibomp5-21is available from the configured LLVM 21 repo and switch the package name accordingly unless there is a strong reason to keep the unversioned runtime.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
.clang-tidy(1 hunks).github/workflows/build.yml(5 hunks).github/workflows/old-compilers.yml(1 hunks)CMakeLists.txt(7 hunks)art_internal_impl.hpp(2 hunks)benchmark/micro_benchmark_node_utils.hpp(1 hunks)fuzz_deepstate/test_art_fuzz_deepstate.cpp(2 hunks)fuzz_deepstate/test_qsbr_fuzz_deepstate.cpp(2 hunks)global.hpp(2 hunks)qsbr.cpp(2 hunks)test/test_key_encode_decode.cpp(2 hunks)test/test_qsbr_ptr.cpp(1 hunks)test_heap.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
global.hpp (1)
art_internal_impl.hpp (1)
UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS(1469-1469)
test/test_key_encode_decode.cpp (1)
portability_builtins.hpp (1)
bswap(25-25)
⏰ 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: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (13)
test_heap.cpp (1)
45-48: Narrow misc-const-correctness suppression is fine here.Extending the existing NOLINT to cover
misc-const-correctnesson themallocresult is tightly scoped and consistent with the existing malloc-related suppressions.test/test_qsbr_ptr.cpp (1)
311-316: Pointer-arithmetic suppression inQSBRPtrSpan.Cendis acceptable.
&two_chars[1] + 1deliberately forms a one-past-end pointer for comparison, and the NOLINT is tightly scoped to that single assertion; no further change seems warranted given the libstdc++ debug-mode constraint mentioned above..github/workflows/old-compilers.yml (1)
472-517: clang 20 entries mirror the existing clang 19 matrix correctly.The new clang 20 jobs match the clang 19 pattern (BUILD_TYPE, sanitizers, COMPILER, VERSION) and should integrate cleanly with the shared setup steps.
Please confirm that the old-compilers workflow actually succeeds with VERSION=20 on the current Ubuntu image (i.e., that the llvm-toolchain repo exposes clang-20 and friends for jammy) by running this workflow branch end-to-end.
test/test_key_encode_decode.cpp (1)
676-683: Pointer-arithmetic cleanup in key tests looks correct.Switching to
std::copy_n(kv.data(), sz, a.begin())and using&kv[sz + 1]for the pad-length memcpy preserves semantics while eliminating the previous bounds suppressions; given the earlier size expectations onkv, there’s no new edge case introduced.Also applies to: 703-707
fuzz_deepstate/test_qsbr_fuzz_deepstate.cpp (1)
614-615: ScopedArrayBoundsuppression around QSBR fuzz test is reasonable.The NOLINT block is constrained to this fuzz test only; as long as the array/indexing logic in the body has been manually audited, this is an acceptable way to silence the noisy analyzer warnings.
Please double-check the specific array / container accesses that triggered the warning to ensure this suppression isn’t hiding a real bug.
Also applies to: 761-762
benchmark/micro_benchmark_node_utils.hpp (1)
231-243: Explicitstd::vector<std::uint64_t>return type is appropriate.This makes the interface clearer without changing behavior; existing call sites already treat the result as a vector. The function still assumes
number_to_key_fnis monotonic non-decreasing ini, which is satisfied by the in-file generators..github/workflows/build.yml (1)
102-159: clang 21 matrix, package selection, and CMake wiring are coherent.Linux Intel/ARM64 clang 21 jobs, the llvm-toolchain-noble-21 repo +
clang-21/clang-tools-21/clang-tidy-21, theV=21CMake wiring, andscan-build-21all line up, so the toolchain versioning is consistent across build and static analysis steps.Please run at least one Linux clang 21 job (both regular and static-analysis variants) on this branch to confirm the noble toolchain repo and the 21.x packages are actually available on the GitHub-hosted runners.
Also applies to: 316-355, 392-395, 404-405, 410-411, 447-447, 500-501
CMakeLists.txt (2)
91-92: Clang‑21 warning flags, STATIC_ANALYSIS tweak, and IWYU workaround are coherentThe CLANG_GE_21_CXX_WARNING_FLAGS definition, its use under
$<AND:${is_standalone},${is_clang_ge_21_not_windows}>, and the extra-Wno-unknown-warning-optionfor STATIC_ANALYSIS and IWYU form a consistent story; I don’t see any mis-wiring or obvious pitfalls here. Just make sure you’ve actually run a STATIC_ANALYSIS build and an IWYU run with clang 21 to confirm these options silence only the intended noise.Also applies to: 334-338, 696-699, 733-733
420-426: Darwin Homebrew libc++ link flags are now scoped to upstream clang‑21 and correct archThe new
cxx_ge_21,is_clang_ge_21_not_windows, andis_darwin_clang_ge_21_{x86_64,arm64}predicates correctly limit the extra-L/-rpathentries to non‑Apple Clang ≥ 21 on macOS, splitting/usr/local/opt/llvm/lib/c++vs/opt/homebrew/opt/llvm/lib/c++by architecture. This fixes the prior over‑broad${is_clang_genex}condition without introducing obvious regressions.Also applies to: 746-749
.clang-tidy (1)
19-22: Disabled clang-tidy checks match the documented rationaleThe comment bullets for
modernize-use-scoped-lockandllvm-prefer-static-over-anonymous-namespaceline up with theChecks:string, andcppcoreguidelines-pro-bounds-pointer-arithmeticis no longer globally suppressed as per the PR goals. No further changes needed here.qsbr.cpp (1)
234-235: NRVO suppression aroundqsbr::register_threadis correctly scopedThe
UNODB_DETAIL_DISABLE_CLANG_21_WARNING("-Wnrvo")/UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS()pair wraps onlyqsbr::register_thread, which is the hot NRVO site, and will be a no-op on non‑clang or pre‑21 compilers. This is a clean, localized way to quiet the clang‑21 warning.Also applies to: 283-283
art_internal_impl.hpp (1)
1447-1470: Clang‑21 NRVO suppression onleave_last_childis appropriately localizedDisabling
-Wnrvojust forleave_last_childand restoring immediately afterwards mirrors the qsbr change and keeps the rest of the ART implementation under full clang‑21 diagnostics. No functional impact, and the scope is as tight as it should be.global.hpp (1)
95-102: Clang‑21-specific warning macros integrate cleanly with existing infrastructureThe documented
UNODB_DETAIL_DISABLE_CLANG_21_WARNING/UNODB_DETAIL_RESTORE_CLANG_21_WARNINGSmacros are correctly implemented: for__clang__with__clang_major__ >= 21they reuse the generic push/pop helpers, and they’re compiled away elsewhere. That makes their use inqsbr.cppandart_internal_impl.hppsafe across non‑clang and older‑clang builds while automatically covering future Clang ≥ 22.Also applies to: 356-362
laurynas-biveinis
force-pushed
the
llvm-21
branch
from
5d1701c to
0c7094e
Compare
- Enable new clang 21 warnings, disable inline as needed - Enable -fsanitize=vptr under UBSan for clang 21 - On macOS and Homebrew-installed LLVM 21, use Homebrew-packaged libc++ instead of XCode standard C++ library for linking - Replace some auto return types with spelled out ones - Disable modernize-use-scoped-lock and llvm-prefer-static-over-anonymous-namespaces clang-tidy checks globally - No longer disable cppcoreguidelines-pro-bounds-pointer-arithmetic check globally, instead suppress it inline where needed - Bump clang version to 21 in GitHub Actions build.yml - Add clang 20 to GitHub Actions old-compilers.yml
laurynas-biveinis
force-pushed
the
llvm-21
branch
from
0c7094e to
3289abb
Compare
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)
fuzz_deepstate/test_qsbr_fuzz_deepstate.cpp (1)
614-762: ArrayBound suppression is acceptable but lacks explanation.The NOLINTBEGIN/END block correctly wraps the QSBR fuzz test to suppress
clang-analyzer-security.ArrayBoundwarnings. Adding a brief comment explaining why this is a known false positive would improve code clarity for future maintainers.
♻️ Duplicate comments (2)
fuzz_deepstate/test_art_fuzz_deepstate.cpp (1)
204-312: ArrayBound suppression is acceptable but lacks explanation.The NOLINTBEGIN/END block correctly wraps the fuzz test to suppress
clang-analyzer-security.ArrayBoundwarnings. However, adding a brief comment explaining why this is a known false positive (e.g., "DeepState controls buffer access bounds" or similar) would improve clarity for future readers.CMakeLists.txt (1)
301-311: Critical: Line 306 still clobbers sanitizer LD flags despite past review.Lines 301-303 and 308-310 correctly add vptr sanitizer support for clang ≥ 21. However, line 306 uses
set(SANITIZER_LD_FLAGS "-fsanitize=undefined")which overwrites any flags previously appended by SANITIZE_ADDRESS or SANITIZE_THREAD blocks. When multiple sanitizers are enabled, you'll compile with all of them but link only with UBSan, causing missing runtime symbols.A past review (marked as addressed in commits 351b86d to 0c7094e) correctly identified this and recommended changing line 306 to
list(APPEND SANITIZER_LD_FLAGS "-fsanitize=undefined"), but the current code still showsset(). Either the fix wasn't applied or there's a regression.- set(SANITIZER_LD_FLAGS "-fsanitize=undefined") + list(APPEND SANITIZER_LD_FLAGS "-fsanitize=undefined")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
.clang-tidy(1 hunks).github/workflows/build.yml(5 hunks).github/workflows/old-compilers.yml(1 hunks)CMakeLists.txt(7 hunks)art_internal_impl.hpp(2 hunks)benchmark/micro_benchmark_node_utils.hpp(1 hunks)fuzz_deepstate/test_art_fuzz_deepstate.cpp(2 hunks)fuzz_deepstate/test_qsbr_fuzz_deepstate.cpp(2 hunks)global.hpp(2 hunks)qsbr.cpp(2 hunks)test/test_key_encode_decode.cpp(1 hunks)test/test_qsbr_ptr.cpp(1 hunks)test_heap.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
global.hpp (1)
art_internal_impl.hpp (1)
UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS(1469-1469)
test/test_key_encode_decode.cpp (1)
portability_builtins.hpp (1)
bswap(25-25)
qsbr.cpp (1)
art_internal_impl.hpp (1)
UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS(1469-1469)
⏰ 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: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (17)
test/test_qsbr_ptr.cpp (1)
311-316: Targeted pointer‑arithmetic suppression is fineThe added NOLINT is narrowly scoped, justified by the libstdc++ debug-mode comment, and aligns with the decision to handle
cppcoreguidelines-pro-bounds-pointer-arithmeticinline instead of globally. No code‑behavior impact..clang-tidy (1)
19-22: Config and rationale for disabled checks are consistentThe comment block for
modernize-use-scoped-lockandllvm-prefer-static-over-anonymous-namespacematches the actualChecks:entry, andcppcoreguidelines-pro-bounds-pointer-arithmeticis no longer globally disabled. This aligns with the stated clang‑tidy strategy..github/workflows/old-compilers.yml (1)
472-517: Clang 20 matrix entries correctly mirror existing clang 19 setupThe added clang‑20 jobs follow the same pattern (build types, sanitizers, env wiring) as the clang‑19 entries and integrate cleanly with the existing
VERSION‑driven install logic. No issues spotted.test/test_key_encode_decode.cpp (1)
703-707: Safer expression for memcpy source, same semanticsSwitching from
kv.data() + sz + 1to&kv[sz + 1]keeps the same address, and the earlier size check (kv.size() == sz + 1 + sizeof(st)) guarantees the indexed range is within bounds. This removes the need for a pointer‑arithmetic suppression without changing behavior.benchmark/micro_benchmark_node_utils.hpp (1)
231-243: Explicit return type forgenerate_keys_to_limitis appropriateChanging the return type from
autotostd::vector<std::uint64_t>matches the actual implementation, keeps existing call sites working, and makes the helper’s contract clearer. No functional or ABI concerns.global.hpp (1)
95-102: Clang‑21 warning macros are well‑scoped and safeThe new
UNODB_DETAIL_DISABLE_CLANG_21_WARNING/UNODB_DETAIL_RESTORE_CLANG_21_WARNINGSare correctly documented and gated on__clang__ && __clang_major__ >= 21, degrading to no‑ops elsewhere. On clang‑21+ they reuse the standard push/pop machinery, which is exactly what you want for narrowly scoping things like-Wnrvo.Also applies to: 356-362
test_heap.cpp (1)
44-47: Additionalmisc-const-correctnesssuppression is acceptable hereThis
operator newis intentionally usingmallocand aconstlocal pointer; extending the NOLINT list to covermisc-const-correctnesskeeps the test helper quiet without touching behavior.art_internal_impl.hpp (1)
1447-1469: Clang‑21 NRVO suppression aroundleave_last_childis correctly scopedWrapping
leave_last_childwithUNODB_DETAIL_DISABLE_CLANG_21_WARNING("-Wnrvo")/UNODB_DETAIL_RESTORE_CLANG_21_WARNINGS()targets a single noisy function, and thanks to the version‑gated macros it only affects clang‑21+. The function’s logic and invariants are unchanged..github/workflows/build.yml (3)
102-154: Job name updates look good.All Intel Linux clang job names consistently updated from "clang 20" to "clang 21" across Release, Debug, and sanitizer configurations.
316-350: ARM64 job name updates look good.All ARM64 Linux clang job names consistently updated to "clang 21" matching the Intel configuration updates.
447-447: Toolchain version and scan-build path updates are correct.The clang version variable (V=21) and scan-build path (/usr/bin/scan-build-21) properly updated to match LLVM 21.
Also applies to: 500-500
CMakeLists.txt (5)
91-92: Warning flags list is clean.The CLANG_GE_21_CXX_WARNING_FLAGS variable correctly lists 5 clang 21 warning flags without any trailing empty string.
414-420: CMake predicates are correctly scoped.The cxx_ge_21, is_clang_ge_21_not_windows, and Darwin architecture-specific predicates correctly limit scope to clang 21+ and properly distinguish between x86_64 and arm64 architectures.
632-636: clang-tidy unknown warning suppression is properly documented.The comment clearly explains why
-Wno-unknown-warning-optionis needed (clang-tidy may not recognize all clang 21 warning flags), and the suppression is narrowly scoped to the clang-tidy invocation.
731-731: Warning flags application is correctly scoped.The CLANG_GE_21_CXX_WARNING_FLAGS are properly gated to standalone builds using clang 21+ on non-Windows platforms, consistent with other compiler-specific warning flag handling.
744-747: Darwin linker flags are correctly architecture-aware.The linker flags properly distinguish between x86_64 (/usr/local) and arm64 (/opt/homebrew) Homebrew installation paths, and are correctly scoped to clang 21+ on Darwin only. This addresses past review concerns about hardcoded paths and overly broad conditions.
qsbr.cpp (1)
234-283: NRVO warning suppression is appropriately scoped.The function is wrapped with CLANG_21_WARNING suppression macros to silence the
-Wnrvowarning. This is a diagnostic-only change with no functional impact, and the pattern is consistent with similar suppressions in other files (e.g., art_internal_impl.hpp).
| "deb http://apt.llvm.org/noble/ llvm-toolchain-noble-21 main" | ||
| sudo apt-get update | ||
| sudo apt-get install -y clang-20 iwyu | ||
| sudo apt-get install -y clang-21 iwyu | ||
| if: runner.os == 'Linux' && env.COMPILER == 'clang' | ||
|
|
||
| - name: Setup dependencies for Linux LLVM (Release) | ||
| run: sudo apt-get install -y libomp5-20 llvm-20 lld-20 | ||
| run: sudo apt-get install -y libomp5 llvm-21 lld-21 | ||
| if: > | ||
| runner.os == 'Linux' && env.COMPILER == 'clang' | ||
| && env.BUILD_TYPE == 'Release' | ||
|
|
||
| - name: Setup dependencies for Linux LLVM (static analysis) | ||
| run: sudo apt-get install -y clang-tools-20 | ||
| run: sudo apt-get install -y clang-tools-21 | ||
| if: > | ||
| runner.os == 'Linux' && env.COMPILER == 'clang' | ||
| && env.STATIC_ANALYSIS == 'ON' | ||
|
|
||
| - name: Setup dependencies for Linux LLVM (not static analysis) | ||
| run: sudo apt-get install -y clang-tidy-20 | ||
| run: sudo apt-get install -y clang-tidy-21 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if commit 42f61a1 exists and what it changed
echo "=== Checking commit 42f61a1 ==="
git log --all --oneline | grep -i "42f61a1" || echo "Searching for commit..."
git show 42f61a1 --stat 2>/dev/null || echo "Commit 42f61a1 not found in history"
echo ""
echo "=== Current state of libomp in workflow ==="
grep -n "libomp" .github/workflows/build.yml || echo "No libomp references found"
echo ""
echo "=== LLVM version references context ==="
sed -n '390,415p' .github/workflows/build.ymlRepository: laurynas-biveinis/unodb
Length of output: 1345
🌐 Web query:
libomp5 LLVM 21 Ubuntu Noble package name
💡 Result:
Use the versioned package libomp5-21 (runtime) — the development package is libomp-21-dev. [1][2]
Sources: apt.llvm.org package list and Ubuntu llvm-toolchain-21 tracker. [1] [2]
Fix libomp5 to use versioned package for consistency with other LLVM 21 dependencies.
Line 398 installs unversioned libomp5 while other packages are correctly versioned to 21. Change libomp5 to libomp5-21 to maintain consistency.
Note: The original review comment referenced commit 42f61a1 as the resolution, but this commit does not exist in the repository history. The issue remains unfixed.
🤖 Prompt for AI Agents
In .github/workflows/build.yml around lines 392 to 410, the Linux LLVM Release
dependency step installs unversioned libomp5 while all other LLVM 21 packages
are versioned; update that step to install libomp5-21 instead of libomp5 to keep
package versions consistent with llvm-21 and lld-21 and avoid mismatched
ABI/versions during the build.
| # IWYU is not necessarily based on the same version clang is and might not | ||
| # understand newer options | ||
| set(DO_IWYU "${IWYU_EXE};-Wno-unknown-warning-option") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
IWYU workaround comment is adequate but could be more detailed.
The comment explains why -Wno-unknown-warning-option is needed for IWYU. However, a past review (marked as addressed) suggested including clang/IWYU version numbers and a TODO with a reference for when this can be removed. The current comment is brief but functional.
🤖 Prompt for AI Agents
In CMakeLists.txt around lines 694-696, the IWYU workaround comment is too
minimal; update it to mention the clang and IWYU versions that required the
-Wno-unknown-warning-option flag and add a TODO with a reference (issue/PR or
upstream bug link) and a target date or condition for removal; reword the
comment to explain that IWYU may use a different clang version which can emit
unknown-warning-option warnings, include the specific versions tested (e.g.,
clang X.Y and iwyu A.B) and add "TODO: remove when IWYU upgrades to >= <version>
or when <link-to-issue> is fixed" so future maintainers know when to revisit
this workaround.
|
1 participant
of XCode standard C++ library for linking
llvm-prefer-static-over-anonymous-namespaces clang-tidy checks globally
globally, instead suppress it inline where needed
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.