[r-barnes]

Summary:
The vendored xxHash header in RocksDB used std::unreachable() whenever __cplusplus is newer than C++20, so building in C++23 mode against a standard library that lacks it (such as libstdc++ 11) fails with "no member named 'unreachable' in namespace 'std'". It now uses std::unreachable() only when <utility> defines __cpp_lib_unreachable (the check upstream xxHash documents), and otherwise keeps the existing __builtin_unreachable() fallback, so generated code is unchanged on every current build. fbcode/rocksdb/src gets this fix through the RocksDB release process.

---

landed-with-radar-review

Differential Revision: D131078354

[meta-codesync]

@r-barnes has exported this pull request. If you are a Meta employee, you can view the originating Diff in D131078354.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 0.0s.

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 478989b

---

Summary

This is a correct and well-motivated fix. Checking __cpp_lib_unreachable instead of __cplusplus > 202002L is the proper way to detect std::unreachable() availability, matching upstream xxHash's documented approach. The change fixes real build failures with C++23-mode compilers paired with older standard libraries (e.g., libstdc++ 11).

High-severity findings (0):
No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. #include <utility> guard still uses language version, not feature-test macro — util/xxhash.h:18

• Issue: The RocksDB customization block at lines 18-21 that #include <utility> still guards on __cplusplus > 202002L. With the proposed change at line 2075, the XXH_UNREACHABLE macro now checks __cpp_lib_unreachable >= 202202L, but that macro is defined by <utility>. If a compiler is in C++20 mode but the standard library defines __cpp_lib_unreachable as a backport/extension (unlikely but possible), the <utility> include would be skipped, __cpp_lib_unreachable would not be defined, and the code would silently fall through to __builtin_unreachable() — which is safe but inconsistent with the intent.
• Root cause: The include guard and the usage guard were updated independently. They should use the same condition, or the include should be unconditional for C++ (since <utility> is always available in C++).
• Suggested fix: Either change line 18 to also check __cpp_lib_unreachable (chicken-and-egg: you need to include <utility> to get the macro), or simply include <utility> unconditionally in C++ mode so the feature-test macro is available. However, this is a vendored file and the current approach still works correctly: in practice, any compiler with __cpp_lib_unreachable will also report __cplusplus > 202002L, so the include fires. The fallback to __builtin_unreachable() is always safe. This is low practical risk.
• Severity rationale: Medium because the two guards are logically inconsistent, even though in practice they won't diverge on any known compiler. A future toolchain could theoretically expose the gap.

🟢 LOW / NIT

L1. Comment accuracy — util/xxhash.h:18-19

• Issue: After the change, the comment at line 19 says "C++23 standard libraries may have std::unreachable()" which is accurate, but the guard at line 18 still says __cplusplus > 202002L (C++23 language version). The comment and guard are consistent but the semantic has shifted: the include is now needed to detect the feature (via __cpp_lib_unreachable), not just to use it. A more precise comment would be: /* Check for std::unreachable() via __cpp_lib_unreachable */.

L2. Consider upstream sync — util/xxhash.h

• Issue: This is a vendored file with RocksDB-specific customizations. The fix aligns with upstream xxHash's recommended approach for std::unreachable() detection. When upstream xxHash next updates this logic, RocksDB's vendored copy should be synced to pick up the fix natively, at which point the RocksDB customization block (lines 15-22) may become unnecessary.

Cross-Component Analysis

Context	Affected?	Analysis
Normal compilation (C++20)	No	CMAKE_CXX_STANDARD defaults to 20; __cplusplus is not > 202002L, so neither the old nor new code path for std::unreachable() is reached. Falls through to __builtin_unreachable() or __assume(0).
C++23 mode with modern libstdc++/libc++	Yes (safe)	__cpp_lib_unreachable is defined after #include <utility>, so std::unreachable() is used. Same behavior as before.
C++23 mode with old libstdc++ (e.g., 11)	Yes (fixed)	Previously failed to compile because std::unreachable() was selected but doesn't exist. Now correctly falls through to __builtin_unreachable().
MSVC	No	MSVC uses __assume(0) via the _MSC_VER branch, which is earlier in the cascade.
xxhash.cc compilation	Yes	XXH_IMPLEMENTATION is defined, so the customization block is active. Same analysis as above applies.

Positive Observations

• The fix is minimal and surgical — only the detection condition changes, preserving the fallback cascade.
• The change matches the C++ standard's recommended way to detect library features via feature-test macros.
• No behavioral change for any correctly-functioning build; only fixes broken builds.
• The PR description clearly explains the motivation and scope.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[meta-codesync]

This pull request has been merged in 6c2e1c9.