edponce commented on a change in pull request #10349:
URL: https://github.com/apache/arrow/pull/10349#discussion_r703464394
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -53,16 +52,10 @@ class ARROW_EXPORT ElementWiseAggregateOptions : public
FunctionOptions {
bool skip_nulls;
};
-/// Functor to calculate hash of an enum.
-template <typename T, typename R = typename std::underlying_type<T>::type,
- typename = std::enable_if<std::is_enum<T>::value>>
-struct EnumHash {
- R operator()(T val) const { return static_cast<R>(val); }
-};
-
/// Rounding and tie-breaking modes for round compute functions.
/// Additional details and examples are provided in compute.rst.
enum class RoundMode : int8_t {
+ // Note: The HALF values need to be last and the first HALF entry is
HALF_DOWN.
Review comment:
By having the HALF_XXX enums "grouped" to one end of the enum, it allows
for checking if the rounding mode is tie-breaking by simply comparing against
one instead of all the cases.
```
if (rmode >= RoundMode::HALF_DOWN) { ... }
// vs
if (rmode == RoundMode::HALF_DOWN || rmode == RoundMode::HALF_UP || ... 4
more times) { ... }
```
I added asserts in the Round/MRound kernel implementations, but not sure
where else I can place them? Round/MRound constructor?
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -53,16 +52,10 @@ class ARROW_EXPORT ElementWiseAggregateOptions : public
FunctionOptions {
bool skip_nulls;
};
-/// Functor to calculate hash of an enum.
-template <typename T, typename R = typename std::underlying_type<T>::type,
- typename = std::enable_if<std::is_enum<T>::value>>
-struct EnumHash {
- R operator()(T val) const { return static_cast<R>(val); }
-};
-
/// Rounding and tie-breaking modes for round compute functions.
/// Additional details and examples are provided in compute.rst.
enum class RoundMode : int8_t {
+ // Note: The HALF values need to be last and the first HALF entry is
HALF_DOWN.
Review comment:
By having the HALF_XXX enums "grouped" to one end of the enum, it allows
for checking if the rounding mode is tie-breaking by simply comparing against
one instead of all the cases.
```c++
if (rmode >= RoundMode::HALF_DOWN) { ... }
// vs
if (rmode == RoundMode::HALF_DOWN || rmode == RoundMode::HALF_UP || ... 4
more times) { ... }
```
I added asserts in the Round/MRound kernel implementations, but not sure
where else I can place them? Round/MRound constructor?
--
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]
