cyb70289 commented on code in PR #13304:
URL: https://github.com/apache/arrow/pull/13304#discussion_r888637048
##########
cpp/src/arrow/scalar.cc:
##########
@@ -75,15 +75,16 @@ struct ScalarHashImpl {
}
Status Visit(const DayTimeIntervalScalar& s) {
- return StdHash(s.value.days) & StdHash(s.value.milliseconds);
+ return StdHash(s.value.days) && StdHash(s.value.milliseconds);
Review Comment:
To me, `&&` is easier to understand. Just like a bash script, I know LHS
will be evaluated before RHS, and RHS will only be evaluated if LHS is true.
##########
cpp/src/arrow/status.h:
##########
@@ -424,6 +426,22 @@ Status Status::operator&(Status&& s) const noexcept {
}
}
+Status Status::operator&&(const Status& s) const noexcept {
+ if (ok()) {
+ return s;
+ } else {
+ return *this;
+ }
+}
+
+Status Status::operator&&(Status&& s) const noexcept {
Review Comment:
Nit: doesn't it make sense if we combine these two overloads into one
function accepting a value parameter?
```
Status Status::operator&&(Status s) {
if (ok()) {
return s;
} else {
return *this;
}
}
```
As `s` is a direct arugment, `return s` is same as `return std::move(s)` if
RVO doesn't apply (a direct argument or stack variable returned is treated as
rvalue, IIRC).
This function may introduce at most one additonal move to both the `const&`
and `&&` overloads. Maybe even no this overhead if RVO can apply to argument,
or compiler can do some tricks, I'm not sure.
--
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]
