bkietz commented on a change in pull request #10583:
URL: https://github.com/apache/arrow/pull/10583#discussion_r660027725
##########
File path: cpp/src/parquet/statistics.cc
##########
@@ -83,12 +87,25 @@ struct UnsignedCompareHelperBase {
using T = typename DType::c_type;
using UCType = typename std::make_unsigned<T>::type;
- constexpr static T DefaultMin() { return std::numeric_limits<UCType>::max();
}
- constexpr static T DefaultMax() { return
std::numeric_limits<UCType>::lowest(); }
+ static_assert(!std::is_same<T, UCType>::value, "T is unsigned");
+ static_assert(sizeof(T) == sizeof(UCType), "T and UCType not the same size");
+
+ // NOTE: according to the C++ spec, unsigned-to-signed conversion is
+ // implementation-defined if the original value does not fix in the signed
type
Review comment:
```suggestion
// implementation-defined if the original value does not fit in the signed
type
```
##########
File path: cpp/src/parquet/statistics.cc
##########
@@ -83,12 +87,25 @@ struct UnsignedCompareHelperBase {
using T = typename DType::c_type;
using UCType = typename std::make_unsigned<T>::type;
- constexpr static T DefaultMin() { return std::numeric_limits<UCType>::max();
}
- constexpr static T DefaultMax() { return
std::numeric_limits<UCType>::lowest(); }
+ static_assert(!std::is_same<T, UCType>::value, "T is unsigned");
+ static_assert(sizeof(T) == sizeof(UCType), "T and UCType not the same size");
+
+ // NOTE: according to the C++ spec, unsigned-to-signed conversion is
+ // implementation-defined if the original value does not fix in the signed
type
+ // (i.e., two's complement cannot be assumed even on mainstream machines,
+ // because the compiler may decide otherwise). Hence the use of `SafeCopy`
+ // below for deterministic bit-casting.
+ // (see "Integer conversions" in
+ // https://en.cppreference.com/w/cpp/language/implicit_conversion)
+
+ static const T DefaultMin() { return
SafeCopy<T>(std::numeric_limits<UCType>::max()); }
+ static const T DefaultMax() {
+ return SafeCopy<T>(std::numeric_limits<UCType>::lowest());
Review comment:
The lowest value for an unsigned type is 0, I think it'd be better to
make that explicit here
```suggestion
return 0;
```
--
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]
