zeroshade commented on code in PR #38443:
URL: https://github.com/apache/arrow/pull/38443#discussion_r1408446590
##########
cpp/src/arrow/c/bridge.cc:
##########
@@ -260,7 +264,7 @@ struct SchemaExporter {
// Dictionary type: parent struct describes index type,
// child dictionary struct describes value type.
RETURN_NOT_OK(VisitTypeInline(*dict_type.index_type(), this));
- dict_exporter_.reset(new SchemaExporter());
+ dict_exporter_ = std::make_unique<SchemaExporter>();
Review Comment:
i've actually wondered.... is there any particular difference (performance
or otherwise) doing `.reset(new T)` vs `= std::make_unique<T>()`?
##########
cpp/src/arrow/c/bridge.cc:
##########
@@ -566,15 +575,31 @@ struct ArrayExporter {
--n_buffers;
++buffers_begin;
}
+
+ bool need_variadic_buffer_sizes =
+ data->type->id() == Type::BINARY_VIEW || data->type->id() ==
Type::STRING_VIEW;
+ if (need_variadic_buffer_sizes) {
+ ++n_buffers;
+ }
+
export_.buffers_.resize(n_buffers);
std::transform(buffers_begin, data->buffers.end(),
export_.buffers_.begin(),
[](const std::shared_ptr<Buffer>& buffer) -> const void* {
return buffer ? buffer->data() : nullptr;
});
+ if (need_variadic_buffer_sizes) {
+ auto variadic_buffers = util::span(data->buffers).subspan(2);
+ export_.variadic_buffer_sizes_.resize(variadic_buffers.size());
+ for (auto [size, buf] : Zip(export_.variadic_buffer_sizes_,
variadic_buffers)) {
+ size = buf->size();
+ }
Review Comment:
the `size` here is a reference? is there any way for it to be more explicit
that this is a reference in the syntax here?
##########
cpp/src/arrow/c/bridge_test.cc:
##########
@@ -122,7 +125,7 @@ class ReleaseCallback {
private:
ARROW_DISALLOW_COPY_AND_ASSIGN(ReleaseCallback);
- bool called_;
+ bool called_{};
Review Comment:
should this be `{ false };` ? (unless I'm forgetting how the default
initialization works)
##########
cpp/src/arrow/c/bridge.cc:
##########
@@ -1418,9 +1454,7 @@ class ImportedBuffer : public Buffer {
struct ArrayImporter {
explicit ArrayImporter(const std::shared_ptr<DataType>& type)
- : type_(type),
- zero_size_buffer_(std::make_shared<Buffer>(kZeroSizeArea, 0)),
- device_type_(DeviceAllocationType::kCPU) {}
Review Comment:
we lost the default to `DeviceAllocationType::kCPU`, should it be added to
the initialization below?
--
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]
