From 60c6b55621881e1b9e0632e84c552277152999f4 Mon Sep 17 00:00:00 2001
From: Will Ayd <william.ayd@icloud.com>
Date: Thu, 26 Sep 2024 23:33:23 -0400
Subject: [PATCH] feedback

---
 src/nanoarrow/common/array_test.cc  | 30 +++++++++++++++--------------
 src/nanoarrow/common/inline_array.h | 14 ++++++++------
 2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/src/nanoarrow/common/array_test.cc b/src/nanoarrow/common/array_test.cc
index 642a920f0..128c7ea47 100644
--- a/src/nanoarrow/common/array_test.cc
+++ b/src/nanoarrow/common/array_test.cc
@@ -895,9 +895,9 @@ TEST(ArrayTest, ArrayTestAppendToLargeStringArray) {
   ArrowArrayRelease(&array);
 }
 
-template <enum ArrowType ArrowT, typename ValueT,
-          ArrowErrorCode (*AppendFunc)(struct ArrowArray*, ValueT)>
-void TestAppendToInlinedDataViewArray() {
+template <enum ArrowType ArrowT, typename ValueT>
+void TestAppendToInlinedDataViewArray(
+    std::function<ArrowErrorCode(struct ArrowArray*, ValueT)> AppendFunc) {
   struct ArrowArray array;
 
   ASSERT_EQ(ArrowArrayInitFromType(&array, ArrowT), NANOARROW_OK);
@@ -934,9 +934,9 @@ void TestAppendToInlinedDataViewArray() {
   ArrowArrayRelease(&array);
 };
 
-template <enum ArrowType ArrowT, typename ValueT,
-          ArrowErrorCode (*AppendFunc)(struct ArrowArray*, ValueT)>
-void TestAppendToDataViewArray() {
+template <enum ArrowType ArrowT, typename ValueT>
+void TestAppendToDataViewArray(
+    std::function<ArrowErrorCode(struct ArrowArray*, ValueT)> AppendFunc) {
   struct ArrowArray array;
 
   ASSERT_EQ(ArrowArrayInitFromType(&array, ArrowT), NANOARROW_OK);
@@ -1004,17 +1004,17 @@ void TestAppendToDataViewArray() {
 };
 
 TEST(ArrayTest, ArrayTestAppendToBinaryViewArray) {
-  TestAppendToInlinedDataViewArray<NANOARROW_TYPE_STRING_VIEW, struct ArrowStringView,
-                                   ArrowArrayAppendString>();
-  TestAppendToDataViewArray<NANOARROW_TYPE_STRING_VIEW, struct ArrowStringView,
-                            ArrowArrayAppendString>();
+  TestAppendToInlinedDataViewArray<NANOARROW_TYPE_STRING_VIEW, struct ArrowStringView>(
+      ArrowArrayAppendString);
+  TestAppendToDataViewArray<NANOARROW_TYPE_STRING_VIEW, struct ArrowStringView>(
+      ArrowArrayAppendString);
 };
 
 TEST(ArrayTest, ArrayTestAppendToStringViewArray) {
-  TestAppendToInlinedDataViewArray<NANOARROW_TYPE_BINARY_VIEW, struct ArrowBufferView,
-                                   ArrowArrayAppendBytes>();
-  TestAppendToDataViewArray<NANOARROW_TYPE_BINARY_VIEW, struct ArrowBufferView,
-                            ArrowArrayAppendBytes>();
+  TestAppendToInlinedDataViewArray<NANOARROW_TYPE_BINARY_VIEW, struct ArrowBufferView>(
+      ArrowArrayAppendBytes);
+  TestAppendToDataViewArray<NANOARROW_TYPE_BINARY_VIEW, struct ArrowBufferView>(
+      ArrowArrayAppendBytes);
 };
 
 TEST(ArrayTest, ArrayTestAppendToFixedSizeBinaryArray) {
@@ -3489,7 +3489,9 @@ TEST(ArrayViewTest, ArrayViewTestGetStringView) {
       string_view_builder, ArrowArrayViewGetStringUnsafe, get_string_view);
   TestGetFromBinaryView<StringViewBuilder, struct ArrowStringView>(
       string_view_builder, ArrowArrayViewGetStringUnsafe, get_string_view);
+}
 
+TEST(ArrayViewTest, ArrayViewTestGetBinaryView) {
   auto binary_view_builder = BinaryViewBuilder();
   const auto get_buffer_view = [](const struct ArrowBufferView* bv) {
     return bv->data.data;
diff --git a/src/nanoarrow/common/inline_array.h b/src/nanoarrow/common/inline_array.h
index f9228729f..415e7d928 100644
--- a/src/nanoarrow/common/inline_array.h
+++ b/src/nanoarrow/common/inline_array.h
@@ -468,6 +468,8 @@ static inline ArrowErrorCode ArrowArrayAppendDouble(struct ArrowArray* array,
   return NANOARROW_OK;
 }
 
+// Binary views only have two fixed buffers, but be aware that they must also
+// always have more 1 buffer to store variadic buffer sizes (even if there are none)
 #define NANOARROW_BINARY_VIEW_FIXED_BUFFERS 2
 #define NANOARROW_BINARY_VIEW_INLINE_SIZE 12
 #define NANOARROW_BINARY_VIEW_PREFIX_SIZE 4
@@ -504,28 +506,28 @@ static inline int32_t ArrowArrayVariadicBufferCount(struct ArrowArray* array) {
 static inline ArrowErrorCode ArrowArrayAddVariadicBuffers(struct ArrowArray* array,
                                                           int32_t nbuffers) {
   const int32_t n_current_bufs = ArrowArrayVariadicBufferCount(array);
-  const int32_t n_bufs_needed = n_current_bufs + nbuffers;
+  const int32_t nvariadic_bufs_needed = n_current_bufs + nbuffers;
 
   struct ArrowArrayPrivateData* private_data =
       (struct ArrowArrayPrivateData*)array->private_data;
 
   private_data->variadic_buffers = (struct ArrowBuffer*)ArrowRealloc(
-      private_data->variadic_buffers, sizeof(struct ArrowBuffer) * n_bufs_needed);
+      private_data->variadic_buffers, sizeof(struct ArrowBuffer) * nvariadic_bufs_needed);
   if (private_data->variadic_buffers == NULL) {
     return ENOMEM;
   }
   private_data->variadic_buffer_sizes = (int64_t*)ArrowRealloc(
-      private_data->variadic_buffer_sizes, sizeof(int64_t) * n_bufs_needed);
+      private_data->variadic_buffer_sizes, sizeof(int64_t) * nvariadic_bufs_needed);
   if (private_data->variadic_buffer_sizes == NULL) {
     return ENOMEM;
   }
 
-  for (int32_t i = n_current_bufs; i < n_bufs_needed; i++) {
+  for (int32_t i = n_current_bufs; i < nvariadic_bufs_needed; i++) {
     ArrowBufferInit(&private_data->variadic_buffers[i]);
     private_data->variadic_buffer_sizes[i] = 0;
   }
-  private_data->n_variadic_buffers = n_bufs_needed;
-  array->n_buffers = NANOARROW_BINARY_VIEW_FIXED_BUFFERS + 1 + n_bufs_needed;
+  private_data->n_variadic_buffers = nvariadic_bufs_needed;
+  array->n_buffers = NANOARROW_BINARY_VIEW_FIXED_BUFFERS + 1 + nvariadic_bufs_needed;
 
   return NANOARROW_OK;
 }