Repository navigation
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR fixes Arrow C++ builds against the macOS 12.3 SDK by switching the Apple-clang-specific workaround selector from __cpp_lib_bitops to the correct feature-test macro __cpp_lib_int_pow2, which matches the availability of std::bit_width / std::has_single_bit (and avoids incorrectly selecting the std::log2p1 fallback where it no longer exists).
Changes:
- Update preprocessor guards at five call sites to use
__cpp_lib_int_pow2instead of__cpp_lib_bitops. - Add brief explanatory comments indicating the correct feature-test macro for
std::bit_width/std::has_single_bit. - Keep all fallback branches and Apple-clang gating (
__apple_build_version__) unchanged.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/src/parquet/encoder.cc | Uses __cpp_lib_int_pow2 to correctly choose between std::log2p1 and std::bit_width on Apple clang + varying SDKs. |
| cpp/src/parquet/chunker_internal.cc | Fixes the SDK-dependent selection for computing target_bits to avoid selecting missing std::log2p1. |
| cpp/src/arrow/util/rle_encoding_test.cc | Aligns the test’s feature selection with the correct macro so it matches the production behavior across SDKs. |
| cpp/src/arrow/util/bit_util.h | Fixes the Log2 helper’s Apple-clang workaround guard to correctly detect std::bit_width availability. |
| cpp/src/arrow/util/align_util.h | Corrects the guard for std::has_single_bit selection to use __cpp_lib_int_pow2. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped to correcting the feature-test macro used for <bit> APIs on Apple SDKs and match the stated build failure and resolution without introducing behavioral changes beyond the intended conditional selection.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
|
535e955 to
fa11bb2
Compare
|
|
…_pow2 The macOS 12.3 SDK build failed with "no member named 'log2p1' in namespace 'std'". The Apple clang workaround selects std::log2p1 when __cpp_lib_bitops is undefined, but that macro covers popcount and friends, not std::bit_width. Use __cpp_lib_int_pow2, which covers std::bit_width and std::has_single_bit and is undefined on 11.3, defined on 12.3. Signed-off-by: 1fanwang <1fannnw@gmail.com>
fa11bb2 to
d0eb576
Compare
|
|
Rationale for this change
Building Arrow against the macOS 12.3 SDK can fail with:
That SDK provides bit_width but leaves the bitops feature macro undefined. Arrow therefore selects the older log2p1 fallback even though the SDK no longer provides it.
Closes #49841.
What changes are included in this PR?
Use the feature-test macro for integer powers of two at the existing Apple-clang workaround sites. The older SDK fallback and the non-Apple path remain unchanged.
Are these changes tested?
Testing Done
I tested with Apple clang 21 and SDK 27.0; macOS 11.3/12.3 SDKs are unavailable here.
The probe below simulates the reported macro combination. It fails against main's headers and passes on this branch. BUILD_DIR is the configured C++ build directory, with tests and Parquet enabled, bundled dependencies and encryption disabled.
Raw logs
Before:
After:
Preprocessor checks passed for all five guards with the feature macro defined and undefined.
Are there any user-facing changes?
Arrow builds against macOS SDK 12.3 while retaining the older SDK workaround. No public API changes.
New Contributor's Guide |
Contributing Overview |
AI-generated Code Guidance
Was AI used for this PR?
AI assisted with the code, regression tests, and description.
PR code and description written by:
Reviewed before submission by: