bkietz commented on a change in pull request #8536:
URL: https://github.com/apache/arrow/pull/8536#discussion_r512717837
##########
File path: r/src/array_to_vector.cpp
##########
@@ -294,22 +294,25 @@ struct Converter_String : public Converter {
array->offset(), n);
for (int i = 0; i < n; i++, null_reader.Next()) {
Review comment:
Please wrap these loops in a call to `unwind_protect`:
```suggestion
cpp11::unwind_protect([&] {
for (int i = 0; i < n; i++, null_reader.Next()) {
```
##########
File path: r/src/array_to_vector.cpp
##########
@@ -294,22 +294,25 @@ struct Converter_String : public Converter {
array->offset(), n);
for (int i = 0; i < n; i++, null_reader.Next()) {
if (null_reader.IsSet()) {
- SET_STRING_ELT(data, start + i,
cpp11::r_string(string_array->GetString(i)));
+ SET_STRING_ELT(data, start + i,
r_string_from_view(string_array->GetView(i)));
} else {
SET_STRING_ELT(data, start + i, NA_STRING);
}
}
} else {
for (int i = 0; i < n; i++) {
- SET_STRING_ELT(data, start + i,
cpp11::r_string(string_array->GetString(i)));
+ SET_STRING_ELT(data, start + i,
r_string_from_view(string_array->GetView(i)));
}
}
return Status::OK();
}
bool Parallel() const { return false; }
+ inline SEXP r_string_from_view(const arrow::util::string_view& view) const {
Review comment:
This method doesn't need to be explicitly marked `inline` or exposed in
the public interface, so let's make it private and static. `string_view`s are
TriviallyCopyable so it's unnecessary to declare them const reference
```suggestion
private:
static SEXP r_string_from_view(arrow::util::string_view view) {
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
