You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "PragmaTwice (via GitHub)" <gi...@apache.org> on 2023/01/29 07:31:28 UTC
[GitHub] [arrow] PragmaTwice opened a new pull request, #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
PragmaTwice opened a new pull request, #33912:
URL: https://github.com/apache/arrow/pull/33912
It fixes #33911.
------
From #33911:
> https://github.com/apache/arrow/blob/4f1d255f3dc57457e5c78d98c4b76fc523cf9961/cpp/src/arrow/result.h#L369-L375
>
> The parameter `generate_alternative` is passed by univeral forwarding, but `std::forward` is missing to use it.
>
> This may result in incorrect overloading selection or compilation errors, such as a class type with the method `operator()() &&`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091261046
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
> > Additionally, I don't think this should be `G&&` as the intention here is to let the compiler not even materialize `G` (e.g. by allocating an object on the stack and calling some constructor for `G`), but instead just splice the code from the lambda into the inlined code for this function.
>
> I'm not sure what you mean here. `G&&` is a [forwarding reference](https://en.cppreference.com/w/cpp/language/reference#Forwarding_references) according to the C++ reference.
Yes, but why do we want to forward `G` when we want the compiler to completely inline the code it represents?
>
> > This is how predicates are passed to functions like `find_if` in the standard library.
>
> Is this the actual declaration in a standard library implementation, or the abstract signature as shown in the reference?
Yes. This is a common way of passing lambdas that you want to be completely inlined if they are inline at the callsite.
See https://github.com/llvm/llvm-project/blob/main/libcxx/include/__algorithm/find_if.h#L23
> (also, since `find_if` typically calls the predicate several times, it can probably not `std::forward` it)
Again, I don't understand why we should forward it. We only call it, don't store/pass it anywhere.
Another example from the STL is how `std::sort` inlines `Compare __c` everywhere. If you pass a lambda it will inline the lambda, if you pass a function pointer or an `std::function` it will invoke it by following the pointer and so on.
https://github.com/llvm/llvm-project/blob/main/libcxx/include/__algorithm/find_if.h#L23
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091108506
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
return MoveValueUnsafe();
}
- return generate_alternative();
+ return std::forward<G>(generate_alternative)();
Review Comment:
@PragmaTwice a way to completely fix the linked issue (regarding overloading) would be to ensure somehow that `G` is a lambda.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091142888
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
> Additionally, I don't think this should be `G&&` as the intention here is to let the compiler not even materialize `G` (e.g. by allocating an object on the stack and calling some constructor for `G`), but instead just splice the code from the lambda into the inlined code for this function.
I'm not sure what you mean here. `G&&` is a [forwarding reference](https://en.cppreference.com/w/cpp/language/reference#Forwarding_references) according to the C++ reference.
> This is how predicates are passed to functions like `find_if` in the standard library.
Is this the actual declaration in a standard library implementation, or the abstract signature as shown in the reference?
(also, since `find_if` typically calls the predicate several times, it can probably not `std::forward` it)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091106649
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
return MoveValueUnsafe();
}
- return generate_alternative();
+ return std::forward<G>(generate_alternative)();
Review Comment:
Note that from the comment, `generate_alternative` is supposed to be a factory not an actual `G` or parameters that can be forwarded to `G`'s constructor. This is a way to avoid evaluating the parameter when `Result` already contains something.
Functional Programming languages make this kind of lazy parameter (call-by-name) very easy to declare. In scalar this would be `alternative: => T` instead of `alternative: T`.
https://www.scala-lang.org/api/current/scala/Option.html#getOrElse[B%3E:A](default:=%3EB):B
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091329095
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
> It seems the version without the moves (and especially without the copies) would be preferred.
You're right @westonpace. I stand corrected.
The implementation from `optional` you linked does exactly as this PR plus a `static_assert` with a nice error message that we could adopt.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #33912:
URL: https://github.com/apache/arrow/pull/33912#issuecomment-1410119851
Thanks for the contribution @PragmaTwice !
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] PragmaTwice commented on pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #33912:
URL: https://github.com/apache/arrow/pull/33912#issuecomment-1409709656
> Let's add a unit test (below requires including `arrow/util/functional.h`):
>
> ```
> TEST(ResultTest, ValueOrGeneratedMoveOnlyGenerator) {
> Result<MoveOnlyDataType> result = Status::Invalid("");
> internal::FnOnce<MoveOnlyDataType()> alternative_generator = [] {
> return MoveOnlyDataType{kIntElement};
> };
> auto out = std::move(result).ValueOrElse(std::move(alternative_generator));
> EXPECT_EQ(*out.data, kIntElement);
> }
> ```
>
> I'll point out that `ValueOrElse` is not used at all within the code base (other than in unit tests) but this concern seems legitimate.
So thanks! Added.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #33912:
URL: https://github.com/apache/arrow/pull/33912#issuecomment-1409091955
Let's add a unit test (below requires including `arrow/util/functional.h`):
```
TEST(ResultTest, ValueOrGeneratedMoveOnlyGenerator) {
Result<MoveOnlyDataType> result = Status::Invalid("");
internal::FnOnce<MoveOnlyDataType()> alternative_generator = [] {
return MoveOnlyDataType{kIntElement};
};
auto out = std::move(result).ValueOrElse(std::move(alternative_generator));
EXPECT_EQ(*out.data, kIntElement);
}
```
I'll point out that `ValueOrElse` is not used at all within the code base (other than in unit tests) but this concern seems legitimate.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091129026
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
Additionally, I don't think this should be `G&&` as the intention here is to let the compiler not even materialize `G` (e.g. by allocating an object on the stack and calling some constructor for `G`), but instead just splice the code from the lambda into the inlined code for this function.
This is how predicates are passed to functions like `find_if` in the standard library.
```cpp
template< class InputIt, class UnaryPredicate >
constexpr InputIt find_if( InputIt first, InputIt last, UnaryPredicate p );
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091118648
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
return MoveValueUnsafe();
}
- return generate_alternative();
+ return std::forward<G>(generate_alternative)();
Review Comment:
An `enable_if` using `is_invocable_r` would restrict the calls only to generators `G` that return `T`.
[1] https://en.cppreference.com/w/cpp/types/is_invocable
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091279797
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
My full proposal is (1) Remove the `&&` from `G` and (2) add the `enable_if` requirement.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091106649
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
return MoveValueUnsafe();
}
- return generate_alternative();
+ return std::forward<G>(generate_alternative)();
Review Comment:
Note that from the comment, `generate_alternative` is supposed to be a factory not an actual `T` or parameters that can be forwarded to `T`'s constructor. This is a way to avoid evaluating the parameter when `Result` already contains something.
Functional Programming languages make this kind of lazy parameter (call-by-name) very easy to declare. In Scalar this would be `alternative: => T` instead of `alternative: T`.
https://www.scala-lang.org/api/current/scala/Option.html#getOrElse[B%3E:A](default:=%3EB):B
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] PragmaTwice commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091365421
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
Thank for your reviews and suggestions!
But I want to point out that there are some difference between `Result::ValueOrElse` and `std::optional::or_else`: `Result<T>::ValueOrElse` returns a value typed `T`, while `std::optional<T>::or_else` typed `std::optional<T>`, so that the generator in `std::optional::or_else` can returns something like `std::nullopt` and then the `or_else` operations can be "chained" (e.g. `v.and_then().or_else().transform().or_else().value()`) and more expressive to achieve "monadic".
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
Thank for your reviews and suggestions!
But I want to point out that there are some difference between `Result::ValueOrElse` and `std::optional::or_else`: `Result<T>::ValueOrElse` returns a value typed `T`, while `std::optional<T>::or_else` typed `std::optional<T>`, so that the generator in `std::optional::or_else` can returns something like `std::nullopt` and then the `or_else` operations can be "chained" (e.g. `v.and_then().or_else().transform().or_else()`) and more expressive to achieve "monadic".
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou merged pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #33912:
URL: https://github.com/apache/arrow/pull/33912
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091261046
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
> > Additionally, I don't think this should be `G&&` as the intention here is to let the compiler not even materialize `G` (e.g. by allocating an object on the stack and calling some constructor for `G`), but instead just splice the code from the lambda into the inlined code for this function.
>
> I'm not sure what you mean here. `G&&` is a [forwarding reference](https://en.cppreference.com/w/cpp/language/reference#Forwarding_references) according to the C++ reference.
Yes, but why do we want to forward `G` when we want the compiler to completely inline the code it represents?
>
> > This is how predicates are passed to functions like `find_if` in the standard library.
>
> Is this the actual declaration in a standard library implementation, or the abstract signature as shown in the reference?
Yes. This is a common way of passing lambdas that you want to be completely inlined if they are inline at the callsite.
See https://github.com/llvm/llvm-project/blob/main/libcxx/include/__algorithm/find_if.h#L23
> (also, since `find_if` typically calls the predicate several times, it can probably not `std::forward` it)
Again, I don't understand why we should forward it. We only call it, don't store/pass it anywhere.
Another example from the STL is how `std::sort` inlines `Compare __c` everywhere. If you pass a lambda it will inline the lambda, if you pass a function pointer or an `std::function` it will invoke it by following the pointer and so on.
https://github.com/llvm/llvm-project/blob/main/libcxx/include/__algorithm/sort.h#L266
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] PragmaTwice commented on pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #33912:
URL: https://github.com/apache/arrow/pull/33912#issuecomment-1409735665
I notice that a CI job [(R / AMD64 Windows C++ RTools 40 ucrt64)](https://github.com/apache/arrow/actions/runs/4050545764/jobs/6968031026) failed, but I cannot find the specific compilation error after reading the CI log. :rofl:
Could anyone help me? So thanks!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] PragmaTwice commented on pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #33912:
URL: https://github.com/apache/arrow/pull/33912#issuecomment-1409858741
> The log is [here](https://github.com/apache/arrow/actions/runs/4050545764/jobs/6968031026) though the error appears to be a failure to build abseil so I'm pretty sure it's unrelated to your PR.
Got it. Thanks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091106649
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
return MoveValueUnsafe();
}
- return generate_alternative();
+ return std::forward<G>(generate_alternative)();
Review Comment:
Note that from the comment, `generate_alternative` is supposed to be a factory not an actual `T` or parameters that can be forwarded to `T`'s constructor. This is a way to avoid evaluating the parameter when `Result` already contains something.
Functional Programming languages make this kind of lazy parameter (call-by-name) very easy to declare. In scalar this would be `alternative: => T` instead of `alternative: T`.
https://www.scala-lang.org/api/current/scala/Option.html#getOrElse[B%3E:A](default:=%3EB):B
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091263716
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
This is becoming pedantic. Do we want to solve #33911 or not?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091278919
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
Sorry. In my other comment I gave a suggestion on how to solve the issue.
Proof that it works as intended: https://godbolt.org/z/5c17zjG89
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091313102
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
[An even more applicable example](https://en.cppreference.com/w/cpp/utility/optional/or_else)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #33912:
URL: https://github.com/apache/arrow/pull/33912#issuecomment-1408315753
@bkietz @felipecrv @benibus Does this look ok?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091264021
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
(and allow for https://github.com/apache/arrow/pull/33912#issuecomment-1409091955)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091106649
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
return MoveValueUnsafe();
}
- return generate_alternative();
+ return std::forward<G>(generate_alternative)();
Review Comment:
Note that from the comment, `generate_alternative` is supposed to be a factory not an actual `T` or parameters that can be forwarded to `T`'s constructor. This is a way to avoid evaluating the parameter when `Result` already contains something.
Functional Programming languages make this kind of lazy parameter (call-by-name) very easy to declare. In Scalar this would be `alternative: => T` instead of `alternative: T`.
https://www.scala-lang.org/api/current/scala/Option.html#getOrElse[B%3E:A](default:=%3EB):B
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091108506
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
return MoveValueUnsafe();
}
- return generate_alternative();
+ return std::forward<G>(generate_alternative)();
Review Comment:
@PragmaTwice a way to completely fix the linked issue would be to ensure somehow that `G` is a lambda.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091108506
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
return MoveValueUnsafe();
}
- return generate_alternative();
+ return std::forward<G>(generate_alternative)();
Review Comment:
@PragmaTwice a way to fix the linked issue would be to ensure somehow that `G` is a lambda.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091261046
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
> > Additionally, I don't think this should be `G&&` as the intention here is to let the compiler not even materialize `G` (e.g. by allocating an object on the stack and calling some constructor for `G`), but instead just splice the code from the lambda into the inlined code for this function.
>
> I'm not sure what you mean here. `G&&` is a [forwarding reference](https://en.cppreference.com/w/cpp/language/reference#Forwarding_references) according to the C++ reference.
Yes, but why do we want to forward `G` when we want the compiler to completely inline the code it represents?
>
> > This is how predicates are passed to functions like `find_if` in the standard library.
>
> Is this the actual declaration in a standard library implementation, or the abstract signature as shown in the reference?
Yes. This is a common way of passing lambdas that you want to be completely inlined if they are inline at the callsite.
See https://github.com/llvm/llvm-project/blob/main/libcxx/include/__algorithm/find_if.h#L23
> (also, since `find_if` typically calls the predicate several times, it can probably not `std::forward` it)
Again, I don't understand why we should forward it. We only call it, don't store/pass it anywhere.
Another example from the STL is how `std::sort` inlines `Compare __c` everywhere. If you pass a lambda it will inline the lambda, if you pass a function pointer or an `std::function` it will invoke it by following the pointer and so on.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091309848
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
> Proof that it works as intended: https://godbolt.org/z/5c17zjG89
I'm a little confused. In this example I get:
```
function &:
copy
copy
function && (with std::move):
move
move
```
If I change to `G&&` I get:
```
function &:
function && (with std::move):
```
It seems the version without the moves (and especially without the copies) would be preferred.
Also, if your generator is stateful (e.g. a generator object that returns the next id in a sequence) then it would make a big difference if you use & or &&.
I agree, in this case, it seems rather unlikely that someone is going to be passing in a stateful (or complex to copy, can't be inlined) generator. Still, I don't think there is any penalty to the &&. And the standard [does sometimes pass functions by forwarding reference](https://en.cppreference.com/w/cpp/utility/functional/invoke).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] PragmaTwice commented on a diff in pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #33912:
URL: https://github.com/apache/arrow/pull/33912#discussion_r1091365421
##########
cpp/src/arrow/result.h:
##########
@@ -371,7 +371,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
if (ok()) {
Review Comment:
Thank for your reviews and suggestions!
But I want to point out that there are some difference between `Result::ValueOrElse` and `std::optional::or_else`: `Result<T>::ValueOrElse` returns a value typed `T`, while `std::optional<T>::or_else` typed `std::optional<T>`, so that the generator in `std::optional::or_else` can returns something like `std::nullopt` and then the `or_else` operations can be "chained" (e.g. `v.and_then().or_else().transform()`) and more expressive to achieve "monadic".
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #33912:
URL: https://github.com/apache/arrow/pull/33912#issuecomment-1410461656
Benchmark runs are scheduled for baseline = dd80f857913a0a81de65179e228fe60b00af4313 and contender = 7a3c66ac8063bc2b2fe184cc02b1750565421f01. 7a3c66ac8063bc2b2fe184cc02b1750565421f01 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/9108035e2bb34f1fa8f4d6305003f703...49b24d827c6a4c9e9058337f6c7fad26/)
[Failed :arrow_down:1.13% :arrow_up:0.06%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/0c9730c1779a40119bb2be0f68c07687...ac33fa01a362416e92ff582c4ba8e482/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/0ee06b4f702343049c2a8f30856cf775...661eed9fc5a8462aac5d77d4a4618e41/)
[Finished :arrow_down:0.16% :arrow_up:0.03%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/401a9808a455477c8665e5d3fce13c48...3f5bc3fc03d64caca596fbf28ec315e7/)
Buildkite builds:
[Finished] [`7a3c66ac` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2292)
[Failed] [`7a3c66ac` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2318)
[Failed] [`7a3c66ac` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2289)
[Finished] [`7a3c66ac` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2310)
[Finished] [`dd80f857` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2291)
[Failed] [`dd80f857` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2317)
[Failed] [`dd80f857` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2288)
[Finished] [`dd80f857` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2309)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #33912: GH-33911: [C++] Add missing std::forward to Result::ValueOrElse
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #33912:
URL: https://github.com/apache/arrow/pull/33912#issuecomment-1409750375
The log is [here](https://github.com/apache/arrow/actions/runs/4050545764/jobs/6968031026) though the error appears to be a failure to build abseil so I'm pretty sure it's unrelated to your PR.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org