zanmato1984 commented on code in PR #51270:
URL: https://github.com/apache/arrow/pull/51270#discussion_r3995009383
##########
cpp/src/arrow/scalar.h:
##########
@@ -172,8 +172,8 @@ struct ArraySpanFillFromScalarScratchSpace {
};
struct ARROW_EXPORT PrimitiveScalarBase : public Scalar {
- explicit PrimitiveScalarBase(std::shared_ptr<DataType> type)
- : Scalar(std::move(type), false) {}
+ explicit PrimitiveScalarBase(const std::shared_ptr<DataType>& type)
Review Comment:
This constructor is an ownership sink rather than a borrowing function.
`Scalar` retains `type`, and `MakeScalarImpl::Visit` currently passes
`std::move(type_)` here.
With the previous value parameter, that ownership could be transferred
through the constructor chain using moves. With `const
std::shared_ptr<DataType>&`, the rvalue binds to the reference, but
`Scalar(type, ...)` must then copy it into its value parameter. This introduces
a reference-count increment and decrement that the previous code avoided.
Could this parameter remain by value and be moved into `Scalar`?
##########
cpp/src/arrow/json/reader.cc:
##########
@@ -370,11 +370,11 @@ class StreamingReaderImpl : public StreamingReader {
}
static Future<std::shared_ptr<StreamingReaderImpl>> MakeAsync(
- std::shared_ptr<DecodeContext> context, std::shared_ptr<io::InputStream>
stream,
+ const std::shared_ptr<DecodeContext>& context, const
std::shared_ptr<io::InputStream>& stream,
Review Comment:
`context` is ultimately owned by the returned reader pipeline, so this is an
ownership-transfer path rather than a pure borrow.
The caller passes a newly created `shared_ptr<DecodeContext>`. Previously,
the value parameter received that temporary and `context = std::move(context)`
transferred it into the capture without a reference-count operation. After
changing the parameter to `const&`, `context = context` must copy the
`shared_ptr`.
Could this remain a value parameter with a move capture?
--
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]
