Copilot commented on code in PR #50998:
URL: https://github.com/apache/arrow/pull/50998#discussion_r4526048446
##########
cpp/src/arrow/util/bit_util.h:
##########
@@ -142,12 +142,14 @@ static inline uint64_t TrailingBits(uint64_t v, int
num_bits) {
//
// The libc++ in the macOS 11.3 SDK used by CRAN only has the pre-standard
// name std::log2p1. __apple_build_version__ should be defined only on Apple
-// clang.
+// clang. The macOS 12 SDKs have std::bit_width but leave __cpp_lib_bitops
+// undefined, so the check uses __cpp_lib_int_pow2, the feature-test macro for
+// std::bit_width and std::has_single_bit.
// TODO: We can remove this condition once CRAN upgrades its macOS SDK from
11.3.
template <typename T>
constexpr int BitWidth(T x) noexcept {
static_assert(std::is_unsigned_v<T>, "BitWidth requires an unsigned integer
type");
-#if defined(__apple_build_version__) && !defined(__cpp_lib_bitops)
+#if defined(__apple_build_version__) && !defined(__cpp_lib_int_pow2)
return static_cast<int>(std::log2p1(x));
#else
return static_cast<int>(std::bit_width(x));
Review Comment:
Feature-test macros are typically compared by value (e.g.,
`__cpp_lib_int_pow2 >= 202002L`) rather than only checking whether they’re
defined. Using a value check would make the guard more robust in case a
toolchain defines `__cpp_lib_int_pow2` with an older value or partial support.
##########
cpp/src/arrow/util/bit_util.h:
##########
@@ -142,12 +142,14 @@ static inline uint64_t TrailingBits(uint64_t v, int
num_bits) {
//
// The libc++ in the macOS 11.3 SDK used by CRAN only has the pre-standard
// name std::log2p1. __apple_build_version__ should be defined only on Apple
-// clang.
+// clang. The macOS 12 SDKs have std::bit_width but leave __cpp_lib_bitops
+// undefined, so the check uses __cpp_lib_int_pow2, the feature-test macro for
+// std::bit_width and std::has_single_bit.
Review Comment:
The comment mentions only the pre-standard `std::log2p1`, but the fallback
also relies on `std::ispow2` in `HasSingleBit`. Please update the comment to
mention both pre-standard names to keep the rationale accurate and complete.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
