zanmato1984 commented on code in PR #48180:
URL: https://github.com/apache/arrow/pull/48180#discussion_r3440368608
##########
cpp/src/arrow/compute/util.cc:
##########
@@ -31,33 +31,33 @@ namespace util {
namespace bit_util {
inline uint64_t SafeLoadUpTo8Bytes(const uint8_t* bytes, int num_bytes) {
- // This will not be correct on big-endian architectures.
-#if !ARROW_LITTLE_ENDIAN
- ARROW_DCHECK(false);
-#endif
ARROW_DCHECK(num_bytes >= 0 && num_bytes <= 8);
if (num_bytes == 8) {
return util::SafeLoad(reinterpret_cast<const uint64_t*>(bytes));
} else {
uint64_t word = 0;
for (int i = 0; i < num_bytes; ++i) {
+#if ARROW_LITTLE_ENDIAN
word |= static_cast<uint64_t>(bytes[i]) << (8 * i);
+#else
+ word |= static_cast<uint64_t>(bytes[i]) << (8 * (num_bytes - 1 - i));
Review Comment:
If this helper is the partial version of `SafeLoad()`, the big-endian branch
should keep native-copy semantics. For `num_bytes < 8`, `p+i` should land in
the same lane as `memcpy(&word, p, num_bytes)`, so the caller-side `ByteSwap()`
can normalize the lanes for bit processing.
```suggestion
word |= static_cast<uint64_t>(bytes[i]) << (8 * (7 - i));
```
##########
cpp/src/arrow/compute/util.cc:
##########
@@ -31,33 +31,33 @@ namespace util {
namespace bit_util {
inline uint64_t SafeLoadUpTo8Bytes(const uint8_t* bytes, int num_bytes) {
- // This will not be correct on big-endian architectures.
-#if !ARROW_LITTLE_ENDIAN
- ARROW_DCHECK(false);
-#endif
ARROW_DCHECK(num_bytes >= 0 && num_bytes <= 8);
if (num_bytes == 8) {
return util::SafeLoad(reinterpret_cast<const uint64_t*>(bytes));
} else {
uint64_t word = 0;
for (int i = 0; i < num_bytes; ++i) {
+#if ARROW_LITTLE_ENDIAN
word |= static_cast<uint64_t>(bytes[i]) << (8 * i);
+#else
+ word |= static_cast<uint64_t>(bytes[i]) << (8 * (num_bytes - 1 - i));
+#endif
}
return word;
}
}
inline void SafeStoreUpTo8Bytes(uint8_t* bytes, int num_bytes, uint64_t value)
{
- // This will not be correct on big-endian architectures.
-#if !ARROW_LITTLE_ENDIAN
- ARROW_DCHECK(false);
-#endif
ARROW_DCHECK(num_bytes >= 0 && num_bytes <= 8);
if (num_bytes == 8) {
util::SafeStore(reinterpret_cast<uint64_t*>(bytes), value);
} else {
for (int i = 0; i < num_bytes; ++i) {
+#if ARROW_LITTLE_ENDIAN
bytes[i] = static_cast<uint8_t>(value >> (8 * i));
+#else
+ bytes[i] = static_cast<uint8_t>(value >> (8 * (num_bytes - 1 - i)));
Review Comment:
Same for the store helper: keep it aligned with native `SafeStore()`
semantics for partial writes.
```suggestion
bytes[i] = static_cast<uint8_t>(value >> (8 * (7 - i)));
```
--
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]
