westonpace commented on a change in pull request #12279:
URL: https://github.com/apache/arrow/pull/12279#discussion_r798191663
##########
File path: cpp/src/arrow/datum.h
##########
@@ -153,6 +153,13 @@ struct ARROW_EXPORT Datum {
: Datum(std::shared_ptr<typename std::conditional<IsArray, Array,
Scalar>::type>(
Review comment:
That looks like the correct invocation to me as well.
The `std::forward` doesn't hurt anything but I don't think it goes far
enough for a correct implementation either (warning, approaching the edge of my
knowledge). Let's consider the simpler version:
```
template <typename T>
Datum(T&& value) : Datum(std::make_shared<T>(std::forward(value))) {}
```
... and some example calls:
```
Int32Scalar value(17);
// In this call `T` resolves to `Int32Scalar&`!
Datum datum_from_lval(value);
// In this call `T` resolves to `Int32Scalar`
Datum datum_from_rval(std::move(value));
```
The second case is fine. In the first case `T` resolves to `Int32Scalar&`
and we end up with `std::make_shared<Int32Scalar&>` which gives us very weird
compile errors.
A potentially correct version might be (warning, please test):
```
template <typename T, typename TV = typename
std::remove_reference<T>::type,
bool IsArray = std::is_base_of<Array, TV>::value,
bool IsScalar = std::is_base_of<Scalar, TV>::value,
typename = enable_if_t<IsArray || IsScalar>>
Datum(T&& value) // NOLINT implicit conversion
: Datum(std::make_shared<TV>(std::forward(value))) {}
```
Some testing: https://godbolt.org/z/cjbPcxMnP
--
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]
