[GitHub] [arrow] bkietz commented on a change in pull request #8536: ARROW-10399: [R] Fix performance regression from cpp11::r_string

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org