chore: Enable more compiler warnings (#351)

As noted in #224, we can use a format check attribute to ensure that format strings/types match and check that `ArrowErrorCode` is always checked. While I was in it for compiler warnings, I enabled a few more that recently caused a problem in the Arrow R package. To minimize impact on existing usage, these are only enabled when `NANOARROW_DEBUG` is defined (as it is for all CMake debug builds and in our CI).
apache · Jan 8, 2024 · be3d432 · be3d432
1 parent 34b0ca5
commit be3d432
Show file tree

Hide file tree

Showing 29 changed files with 211 additions and 118 deletions.
diff --git a/.github/workflows/r-check.yaml b/.github/workflows/r-check.yaml
@@ -60,15 +60,15 @@ jobs:
           http-user-agent: ${{ matrix.config.http-user-agent }}
           use-public-rspm: true
 
-      - name: Install valgrind on r-devel
-        if: matrix.config.r == 'devel'
-        run: sudo apt-get install -y valgrind
-
-      # Until new pkgbuild is on CRAN, we do the install step to ensure
-      # the C library is vendored from this commit
-      - name: Vendor nanoarrow into the R package
+      # Check package install with extra warning flags except on Windows,
+      # where the R headers cause some problems
+      - name: Check R CMD INSTALL
+        if: matrix.config.os != 'windows-latest'
+        env:
+          PKG_CPPFLAGS: "-DNANOARROW_DEBUG"
+          PKG_CFLAGS: "-Werror -Wall -Wextra -Wpedantic -Wconversion -Wno-unused-parameter -Wno-sign-conversion -Wno-cast-function-type"
         run: |
-          R CMD INSTALL r
+          R CMD INSTALL r --preclean
         shell: bash
 
       - uses: r-lib/actions/setup-r-dependencies@v2

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -127,20 +127,24 @@ else()
                              PRIVATE -Wall
                                      -Werror
                                      -Wextra
+                                     -Wpedantic
                                      -Wno-type-limits
                                      -Wno-unused-parameter
                                      -Wmaybe-uninitialized
-                                     -Wpedantic
-                                     -Wunused-result)
+                                     -Wunused-result
+                                     -Wconversion
+                                     -Wno-sign-conversion)
     elseif(CMAKE_C_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_C_COMPILER_ID STREQUAL
                                                         "Clang")
       target_compile_options(nanoarrow
                              PRIVATE -Wall
                                      -Werror
                                      -Wextra
+                                     -Wpedantic
                                      -Wdocumentation
                                      -Wno-unused-parameter
-                                     -Wshorten-64-to-32)
+                                     -Wconversion
+                                     -Wno-sign-conversion)
     endif()
   endif()
 

diff --git a/r/R/buffer.R b/r/R/buffer.R
@@ -42,14 +42,21 @@ as_nanoarrow_buffer.nanoarrow_buffer <- function(x, ...) {
 
 #' @export
 as_nanoarrow_buffer.default <- function(x, ...) {
+  msg <- NULL
   result <- tryCatch(
     .Call(nanoarrow_c_as_buffer_default, x),
-    error = function(...) NULL
+    error = function(e) {
+      msg <<- conditionMessage(e)
+      NULL
+    }
   )
 
-  if (is.null(result)) {
+  if (is.null(result) && is.null(msg)) {
     cls <- paste(class(x), collapse = "/")
     stop(sprintf("Can't convert object of type %s to nanoarrow_buffer", cls))
+  } else if (is.null(result)) {
+    cls <- paste(class(x), collapse = "/")
+    stop(sprintf("Can't convert object of type %s to nanoarrow_buffer: %s", cls, msg))
   }
 
   result

diff --git a/r/src/altrep.c b/r/src/altrep.c
@@ -77,7 +77,7 @@ static SEXP nanoarrow_altstring_elt(SEXP altrep_sexp, R_xlen_t i) {
   }
 
   struct ArrowStringView item = ArrowArrayViewGetStringUnsafe(&converter->array_view, i);
-  return Rf_mkCharLenCE(item.data, item.size_bytes, CE_UTF8);
+  return Rf_mkCharLenCE(item.data, (int)item.size_bytes, CE_UTF8);
 }
 
 static SEXP nanoarrow_altstring_materialize(SEXP altrep_sexp) {

diff --git a/r/src/array.c b/r/src/array.c
@@ -55,7 +55,7 @@ SEXP nanoarrow_c_array_set_length(SEXP array_xptr, SEXP length_sexp) {
     Rf_error("array$length must be finite and greater than zero");
   }
 
-  array->length = length;
+  array->length = (int64_t)length;
   return R_NilValue;
 }
 
@@ -70,7 +70,7 @@ SEXP nanoarrow_c_array_set_null_count(SEXP array_xptr, SEXP null_count_sexp) {
     Rf_error("array$null_count must be finite and greater than -1");
   }
 
-  array->null_count = null_count;
+  array->null_count = (int64_t)null_count;
   return R_NilValue;
 }
 
@@ -85,7 +85,7 @@ SEXP nanoarrow_c_array_set_offset(SEXP array_xptr, SEXP offset_sexp) {
     Rf_error("array$offset must be finite and greater than zero");
   }
 
-  array->offset = offset;
+  array->offset = (int64_t)offset;
   return R_NilValue;
 }
 
@@ -220,8 +220,8 @@ static int move_array_buffers(struct ArrowArray* src, struct ArrowArray* dst,
   dst->offset = src->offset;
 
   if (src->n_buffers != dst->n_buffers) {
-    ArrowErrorSet(error, "Expected %ld buffer(s) but got %ld", dst->n_buffers,
-                  src->n_buffers);
+    ArrowErrorSet(error, "Expected %ld buffer(s) but got %ld", (long)dst->n_buffers,
+                  (long)src->n_buffers);
     return EINVAL;
   }
 
@@ -230,8 +230,8 @@ static int move_array_buffers(struct ArrowArray* src, struct ArrowArray* dst,
   }
 
   if (src->n_children != dst->n_children) {
-    ArrowErrorSet(error, "Expected %ld child(ren) but got %ld", dst->n_children,
-                  src->n_children);
+    ArrowErrorSet(error, "Expected %ld child(ren) but got %ld", (long)dst->n_children,
+                  (long)src->n_children);
     return EINVAL;
   }
 

diff --git a/r/src/array_view.c b/r/src/array_view.c
@@ -39,7 +39,7 @@ SEXP nanoarrow_c_array_view(SEXP array_xptr, SEXP schema_xptr) {
   struct ArrowSchema* schema = nanoarrow_schema_from_xptr(schema_xptr);
 
   struct ArrowError error;
-  ArrowErrorSet(&error, "");
+  ArrowErrorInit(&error);
 
   struct ArrowArrayView* array_view =
       (struct ArrowArrayView*)ArrowMalloc(sizeof(struct ArrowArrayView));

diff --git a/r/src/as_array.c b/r/src/as_array.c
@@ -95,7 +95,7 @@ static void as_array_int(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr
     for (int64_t i = first_null; i < len; i++) {
       uint8_t is_valid = x_data[i] != NA_INTEGER;
       null_count += !is_valid;
-      ArrowBitmapAppend(&bitmap, is_valid, 1);
+      ArrowBitmapAppendUnsafe(&bitmap, is_valid, 1);
     }
 
     ArrowArraySetValidityBitmap(array, &bitmap);
@@ -141,13 +141,16 @@ static void as_array_lgl(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr
   for (int64_t i = 0; i < len; i++) {
     if (x_data[i] == NA_INTEGER) {
       has_nulls = 1;
-      ArrowBitmapAppend(&value_bitmap, 0, 1);
+      ArrowBitmapAppendUnsafe(&value_bitmap, 0, 1);
     } else {
-      ArrowBitmapAppend(&value_bitmap, x_data[i] != 0, 1);
+      ArrowBitmapAppendUnsafe(&value_bitmap, x_data[i] != 0, 1);
     }
   }
 
-  ArrowArraySetBuffer(array, 1, &value_bitmap.buffer);
+  result = ArrowArraySetBuffer(array, 1, &value_bitmap.buffer);
+  if (result != NANOARROW_OK) {
+    Rf_error("ArrowArraySetBuffer() failed");
+  }
 
   // Set the array fields
   array->length = len;
@@ -166,7 +169,7 @@ static void as_array_lgl(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr
     for (int64_t i = 0; i < len; i++) {
       uint8_t is_valid = x_data[i] != NA_INTEGER;
       null_count += !is_valid;
-      ArrowBitmapAppend(&bitmap, is_valid, 1);
+      ArrowBitmapAppendUnsafe(&bitmap, is_valid, 1);
     }
 
     ArrowArraySetValidityBitmap(array, &bitmap);
@@ -219,7 +222,7 @@ static void as_array_dbl(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr
       if (R_IsNA(x_data[i]) || R_IsNaN(x_data[i])) {
         buffer_data[i] = 0;
       } else {
-        buffer_data[i] = x_data[i];
+        buffer_data[i] = (int64_t)x_data[i];
       }
     }
 
@@ -245,7 +248,7 @@ static void as_array_dbl(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr
         n_overflow++;
         buffer_data[i] = 0;
       } else {
-        buffer_data[i] = x_data[i];
+        buffer_data[i] = (int32_t)x_data[i];
       }
     }
 
@@ -283,7 +286,7 @@ static void as_array_dbl(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr
     for (int64_t i = first_null; i < len; i++) {
       uint8_t is_valid = !R_IsNA(x_data[i]) && !R_IsNaN(x_data[i]);
       null_count += !is_valid;
-      ArrowBitmapAppend(&bitmap, is_valid, 1);
+      ArrowBitmapAppendUnsafe(&bitmap, is_valid, 1);
     }
 
     ArrowArraySetValidityBitmap(array, &bitmap);
@@ -316,7 +319,10 @@ static void as_array_chr(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr
   struct ArrowBuffer* offset_buffer = ArrowArrayBuffer(array, 1);
   struct ArrowBuffer* data_buffer = ArrowArrayBuffer(array, 2);
 
-  ArrowBufferReserve(offset_buffer, (len + 1) * sizeof(int32_t));
+  result = ArrowBufferReserve(offset_buffer, (len + 1) * sizeof(int32_t));
+  if (result != NANOARROW_OK) {
+    Rf_error("ArrowBufferReserve() failed");
+  }
 
   int64_t null_count = 0;
   int32_t cumulative_len = 0;
@@ -338,7 +344,7 @@ static void as_array_chr(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr
       if (result != NANOARROW_OK) {
         Rf_error("ArrowBufferAppend() failed");
       }
-      cumulative_len += item_size;
+      cumulative_len += (int32_t)item_size;
 
       vmaxset(vmax);
     }
@@ -361,7 +367,7 @@ static void as_array_chr(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr
 
     for (int64_t i = 0; i < len; i++) {
       uint8_t is_valid = STRING_ELT(x_sexp, i) != NA_STRING;
-      ArrowBitmapAppend(&bitmap, is_valid, 1);
+      ArrowBitmapAppendUnsafe(&bitmap, is_valid, 1);
     }
 
     ArrowArraySetValidityBitmap(array, &bitmap);
@@ -470,7 +476,7 @@ static void as_array_list(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xpt
       Rf_error("ArrowBufferAppend() failed");
     }
 
-    cumulative_len += item_size;
+    cumulative_len += (int32_t)item_size;
     ArrowBufferAppendUnsafe(offset_buffer, &cumulative_len, sizeof(int32_t));
   }
 
@@ -489,7 +495,7 @@ static void as_array_list(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xpt
 
     for (int64_t i = 0; i < len; i++) {
       uint8_t is_valid = VECTOR_ELT(x_sexp, i) != R_NilValue;
-      ArrowBitmapAppend(&bitmap, is_valid, 1);
+      ArrowBitmapAppendUnsafe(&bitmap, is_valid, 1);
     }
 
     ArrowArraySetValidityBitmap(array, &bitmap);

diff --git a/r/src/buffer.c b/r/src/buffer.c
@@ -64,7 +64,10 @@ SEXP nanoarrow_c_as_buffer_default(SEXP x_sexp) {
       if (x_sexp != NA_STRING) {
         data = CHAR(x_sexp);
         break;
+      } else {
+        Rf_error("NA_character_ not supported in as_nanoarrow_buffer()");
       }
+      break;
     default:
       Rf_error("Unsupported type");
   }
@@ -178,8 +181,8 @@ SEXP nanoarrow_c_buffer_info(SEXP buffer_xptr) {
                          ""};
   SEXP info = PROTECT(Rf_mkNamed(VECSXP, names));
   SET_VECTOR_ELT(info, 0, R_MakeExternalPtr(buffer->data, NULL, buffer_xptr));
-  SET_VECTOR_ELT(info, 1, Rf_ScalarReal(buffer->size_bytes));
-  SET_VECTOR_ELT(info, 2, Rf_ScalarReal(buffer->capacity_bytes));
+  SET_VECTOR_ELT(info, 1, Rf_ScalarReal((double)buffer->size_bytes));
+  SET_VECTOR_ELT(info, 2, Rf_ScalarReal((double)buffer->capacity_bytes));
   SET_VECTOR_ELT(info, 3, buffer_type_sexp);
   SET_VECTOR_ELT(info, 4, buffer_data_type_sexp);
   SET_VECTOR_ELT(info, 5, Rf_ScalarInteger(element_size_bits));
@@ -189,7 +192,7 @@ SEXP nanoarrow_c_buffer_info(SEXP buffer_xptr) {
 
 SEXP nanoarrow_c_buffer_head_bytes(SEXP buffer_xptr, SEXP max_bytes_sexp) {
   struct ArrowBuffer* buffer = buffer_from_xptr(buffer_xptr);
-  int64_t max_bytes = REAL(max_bytes_sexp)[0];
+  int64_t max_bytes = (int64_t)REAL(max_bytes_sexp)[0];
 
   if (buffer->size_bytes <= max_bytes) {
     return buffer_xptr;

diff --git a/r/src/buffer.h b/r/src/buffer.h
@@ -72,11 +72,11 @@ static inline SEXP buffer_borrowed_xptr(const void* addr, int64_t size_bytes,
 static inline void buffer_borrowed_xptr_set_type(SEXP buffer_xptr,
                                                  enum ArrowBufferType buffer_type,
                                                  enum ArrowType buffer_data_type,
-                                                 int32_t element_size_bits) {
+                                                 int64_t element_size_bits) {
   SEXP buffer_types_sexp = PROTECT(Rf_allocVector(INTSXP, 3));
   INTEGER(buffer_types_sexp)[0] = buffer_type;
   INTEGER(buffer_types_sexp)[1] = buffer_data_type;
-  INTEGER(buffer_types_sexp)[2] = element_size_bits;
+  INTEGER(buffer_types_sexp)[2] = (int32_t)element_size_bits;
   R_SetExternalPtrTag(buffer_xptr, buffer_types_sexp);
   UNPROTECT(1);
 }

diff --git a/r/src/convert_array_stream.c b/r/src/convert_array_stream.c
@@ -30,8 +30,15 @@ SEXP nanoarrow_c_convert_array_stream(SEXP array_stream_xptr, SEXP ptype_sexp,
                                       SEXP size_sexp, SEXP n_sexp) {
   struct ArrowArrayStream* array_stream =
       nanoarrow_array_stream_from_xptr(array_stream_xptr);
-  double size = REAL(size_sexp)[0];
-  double n = REAL(n_sexp)[0];
+  int64_t size = (int64_t)(REAL(size_sexp)[0]);
+
+  double n_real = REAL(n_sexp)[0];
+  int n;
+  if (R_FINITE(n_real)) {
+    n = (int)n_real;
+  } else {
+    n = INT_MAX;
+  }
 
   SEXP schema_xptr = PROTECT(nanoarrow_schema_owning_xptr());
   struct ArrowSchema* schema = nanoarrow_output_schema_from_xptr(schema_xptr);

diff --git a/r/src/materialize.c b/r/src/materialize.c
@@ -89,11 +89,11 @@ void nanoarrow_set_rownames(SEXP x, R_xlen_t len) {
   if (len <= INT_MAX) {
     SEXP rownames = PROTECT(Rf_allocVector(INTSXP, 2));
     INTEGER(rownames)[0] = NA_INTEGER;
-    INTEGER(rownames)[1] = -len;
+    INTEGER(rownames)[1] = (int)(-len);
     Rf_setAttrib(x, R_RowNamesSymbol, rownames);
     UNPROTECT(1);
   } else {
-    SEXP length_dbl = PROTECT(Rf_ScalarReal(len));
+    SEXP length_dbl = PROTECT(Rf_ScalarReal((double)len));
     SEXP seq_len_symbol = PROTECT(Rf_install("seq_len"));
     SEXP seq_len_call = PROTECT(Rf_lang2(seq_len_symbol, length_dbl));
     SEXP rownames_call = PROTECT(Rf_lang2(R_AsCharacterSymbol, seq_len_call));
@@ -294,9 +294,9 @@ static int nanoarrow_materialize_other(struct RConverter* converter,
       (struct ArrowArray*)converter->array_view.array, schema_xptr, converter_xptr));
   Rf_setAttrib(array_xptr, R_ClassSymbol, nanoarrow_cls_array);
 
-  SEXP offset_sexp =
-      PROTECT(Rf_ScalarReal(converter->src.array_view->offset + converter->src.offset));
-  SEXP length_sexp = PROTECT(Rf_ScalarReal(converter->src.length));
+  SEXP offset_sexp = PROTECT(
+      Rf_ScalarReal((double)(converter->src.array_view->offset + converter->src.offset)));
+  SEXP length_sexp = PROTECT(Rf_ScalarReal((double)converter->src.length));
 
   SEXP fun = PROTECT(Rf_install("convert_fallback_other"));
   SEXP call = PROTECT(

diff --git a/r/src/materialize_chr.h b/r/src/materialize_chr.h
@@ -79,7 +79,7 @@ static inline int nanoarrow_materialize_chr(struct RConverter* converter) {
     } else {
       item = ArrowArrayViewGetStringUnsafe(src->array_view, src->offset + i);
       SET_STRING_ELT(dst->vec_sexp, dst->offset + i,
-                     Rf_mkCharLenCE(item.data, item.size_bytes, CE_UTF8));
+                     Rf_mkCharLenCE(item.data, (int)item.size_bytes, CE_UTF8));
     }
   }
 

diff --git a/r/src/materialize_common.h b/r/src/materialize_common.h
@@ -109,7 +109,7 @@ struct RConverter {
 
 static inline void warn_lossy_conversion(int64_t count, const char* msg) {
   SEXP fun = PROTECT(Rf_install("warn_lossy_conversion"));
-  SEXP count_sexp = PROTECT(Rf_ScalarReal(count));
+  SEXP count_sexp = PROTECT(Rf_ScalarReal((double)count));
   SEXP msg_sexp = PROTECT(Rf_mkString(msg));
   SEXP call = PROTECT(Rf_lang3(fun, count_sexp, msg_sexp));
   Rf_eval(call, nanoarrow_ns_pkg);

diff --git a/r/src/materialize_int.h b/r/src/materialize_int.h
@@ -80,7 +80,7 @@ static inline int nanoarrow_materialize_int(struct ArrayViewSlice* src,
       // No need to bounds check for these types
       for (R_xlen_t i = 0; i < dst->length; i++) {
         result[dst->offset + i] =
-            ArrowArrayViewGetIntUnsafe(src->array_view, src->offset + i);
+            (int32_t)ArrowArrayViewGetIntUnsafe(src->array_view, src->offset + i);
       }
 
       // Set any nulls to NA_INTEGER
@@ -107,7 +107,7 @@ static inline int nanoarrow_materialize_int(struct ArrayViewSlice* src,
               result[dst->offset + i] = NA_INTEGER;
               n_bad_values++;
             } else {
-              result[dst->offset + i] = value;
+              result[dst->offset + i] = (int32_t)value;
             }
           } else {
             result[dst->offset + i] = NA_INTEGER;
@@ -120,7 +120,7 @@ static inline int nanoarrow_materialize_int(struct ArrayViewSlice* src,
             result[dst->offset + i] = NA_INTEGER;
             n_bad_values++;
           } else {
-            result[dst->offset + i] = value;
+            result[dst->offset + i] = (int32_t)value;
           }
         }
       }