chore(ci): Add clang-tidy checks and ensure they pass (#639)

This PR adds a `clang-tidy` check to CI and fixes several issues that it
identified (including a few from other repos like ADBC and cudf).

A reboot of #538; closes #537.

Author: paleolimbot

.clang-tidy

@@ -0,0 +1,23 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+---
+# Disable valist, it's buggy: https://github.com/llvm/llvm-project/issues/40656
+# Disable DeprecatedOrUnsafeBufferHandling because it suggests we replace
+# memset and memcpy with memset_s() and memcpy_s() if compiled with C11. Because
+# we also support C99, we can't blindly replace those calls.
+Checks: '-clang-analyzer-valist.Uninitialized,-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling'
+FormatStyle: google

.github/workflows/clang-tidy.yaml

@@ -0,0 +1,77 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: clang-tidy
+
+on:
+  push:
+    branches:
+      - main
+  pull_request:
+    branches:
+      - main
+    paths:
+      - 'CMakeLists.txt'
+      - '.github/workflows/clang-tidy.yaml'
+      - 'src/nanoarrow/**'
+
+permissions:
+  contents: read
+
+jobs:
+  clang-tidy:
+
+    runs-on: ubuntu-latest
+
+    name: ${{ matrix.config.label }}
+
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Cache Arrow C++ Build
+        id: cache-arrow-build
+        uses: actions/cache@v4
+        with:
+          path: arrow
+          # Bump the number at the end of this line to force a new Arrow C++ build
+          key: arrow-${{ runner.os }}-${{ runner.arch }}-1
+
+      - name: Build Arrow C++
+        if: steps.cache-arrow-build.outputs.cache-hit != 'true'
+        shell: bash
+        run: |
+          ci/scripts/build-arrow-cpp-minimal.sh 15.0.2 arrow
+
+      - name: Build nanoarrow
+        run: |
+          export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:`pwd`/dist/lib
+          sudo ldconfig
+
+          ARROW_PATH="$(pwd)/arrow"
+          mkdir build
+          cd build
+
+          cmake .. -DNANOARROW_DEVICE=ON -DNANOARROW_IPC=ON \
+            -DNANOARROW_BUILD_TESTS=ON -DCMAKE_POSITION_INDEPENDENT_CODE=ON \
+            -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
+            -DCMAKE_PREFIX_PATH="${ARROW_PATH}"
+
+          cmake --build .
+
+      - name: Run clang-tidy
+        run: |
+          ci/scripts/run-clang-tidy.sh . build/

ci/scripts/run-clang-tidy.sh

@@ -0,0 +1,48 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+set -e
+
+main() {
+    local -r source_dir="${1}"
+    local -r build_dir="${2}"
+
+    if [ $(uname) = "Darwin" ]; then
+      local -r jobs=$(sysctl -n hw.ncpu)
+    else
+      local -r jobs=$(nproc)
+    fi
+
+    set -x
+
+    run-clang-tidy -p "${build_dir}" -j$jobs \
+        -extra-arg=-Wno-unknown-warning-option | \
+        tee "${build_dir}/clang-tidy-output.txt"
+
+    if grep -e "warning:" -e "error:" "${build_dir}/clang-tidy-output.txt"; then
+      echo "Warnings or errors found!"
+      exit 1
+    else
+      echo "No warnings or errors found!"
+    fi
+
+    set +x
+}
+
+main "$@"

src/nanoarrow/common/array.c

@@ -842,7 +842,9 @@ static int ArrowArrayViewValidateMinimal(struct ArrowArrayView* array_view,
   for (int i = 0; i < 2; i++) {
     int64_t element_size_bytes = array_view->layout.element_size_bits[i] / 8;
     // Initialize with a value that will cause an error if accidentally used uninitialized
-    int64_t min_buffer_size_bytes = array_view->buffer_views[i].size_bytes + 1;
+    // Need to suppress the clang-tidy warning because gcc warns for possible use
+    int64_t min_buffer_size_bytes =  // NOLINT(clang-analyzer-deadcode.DeadStores)
+        array_view->buffer_views[i].size_bytes + 1;
 
     switch (array_view->layout.buffer_type[i]) {
       case NANOARROW_BUFFER_TYPE_VALIDITY:

src/nanoarrow/common/inline_buffer.h

@@ -214,6 +214,7 @@ static inline ArrowErrorCode ArrowBufferReserve(struct ArrowBuffer* buffer,
 static inline void ArrowBufferAppendUnsafe(struct ArrowBuffer* buffer, const void* data,
                                            int64_t size_bytes) {
   if (size_bytes > 0) {
+    NANOARROW_DCHECK(buffer->data != NULL);
     memcpy(buffer->data + buffer->size_bytes, data, size_bytes);
     buffer->size_bytes += size_bytes;
   }
@@ -295,8 +296,10 @@ static inline ArrowErrorCode ArrowBufferAppendFill(struct ArrowBuffer* buffer,
 
   NANOARROW_RETURN_NOT_OK(ArrowBufferReserve(buffer, size_bytes));
 
+  NANOARROW_DCHECK(buffer->data != NULL);  // To help clang-tidy
   memset(buffer->data + buffer->size_bytes, value, size_bytes);
   buffer->size_bytes += size_bytes;
+
   return NANOARROW_OK;
 }
 
@@ -413,6 +416,8 @@ static inline void ArrowBitsUnpackInt32(const uint8_t* bits, int64_t start_offse
     return;
   }
 
+  NANOARROW_DCHECK(bits != NULL && out != NULL);
+
   const int64_t i_begin = start_offset;
   const int64_t i_end = start_offset + length;
   const int64_t i_last_valid = i_end - 1;
@@ -461,6 +466,12 @@ static inline void ArrowBitSetTo(uint8_t* bits, int64_t i, uint8_t bit_is_set) {
 
 static inline void ArrowBitsSetTo(uint8_t* bits, int64_t start_offset, int64_t length,
                                   uint8_t bits_are_set) {
+  if (length == 0) {
+    return;
+  }
+
+  NANOARROW_DCHECK(bits != NULL);
+
   const int64_t i_begin = start_offset;
   const int64_t i_end = start_offset + length;
   const uint8_t fill_byte = (uint8_t)(-bits_are_set);
@@ -504,6 +515,8 @@ static inline int64_t ArrowBitCountSet(const uint8_t* bits, int64_t start_offset
     return 0;
   }
 
+  NANOARROW_DCHECK(bits != NULL);
+
   const int64_t i_begin = start_offset;
   const int64_t i_end = start_offset + length;
   const int64_t i_last_valid = i_end - 1;

src/nanoarrow/common/nanoarrow_hpp_test.cc

@@ -50,7 +50,7 @@ TEST(NanoarrowHppTest, NanoarrowHppUniqueArrayTest) {
 
   // move constructor
   nanoarrow::UniqueArray array2 = std::move(array);
-  EXPECT_EQ(array->release, nullptr);
+  EXPECT_EQ(array->release, nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
   EXPECT_NE(array2->release, nullptr);
   EXPECT_EQ(array2->length, 1);
 
@@ -71,7 +71,7 @@ TEST(NanoarrowHppTest, NanoarrowHppUniqueSchemaTest) {
 
   // move constructor
   nanoarrow::UniqueSchema schema2 = std::move(schema);
-  EXPECT_EQ(schema->release, nullptr);
+  EXPECT_EQ(schema->release, nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
   EXPECT_NE(schema2->release, nullptr);
   EXPECT_STREQ(schema2->format, "i");
 
@@ -101,7 +101,7 @@ TEST(NanoarrowHppTest, NanoarrowHppUniqueArrayStreamTest) {
 
   // move constructor
   nanoarrow::UniqueArrayStream array_stream2 = std::move(array_stream);
-  EXPECT_EQ(array_stream->release, nullptr);
+  EXPECT_EQ(array_stream->release, nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
   EXPECT_NE(array_stream2->release, nullptr);
   EXPECT_EQ(ArrowArrayStreamGetSchema(array_stream2.get(), schema.get(), nullptr),
             NANOARROW_OK);
@@ -137,8 +137,8 @@ TEST(NanoarrowHppTest, NanoarrowHppUniqueBufferTest) {
 
   // move constructor
   nanoarrow::UniqueBuffer buffer2 = std::move(buffer);
-  EXPECT_EQ(buffer->data, nullptr);
-  EXPECT_EQ(buffer->size_bytes, 0);
+  EXPECT_EQ(buffer->data, nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
+  EXPECT_EQ(buffer->size_bytes, 0);  // NOLINT(clang-analyzer-cplusplus.Move)
   EXPECT_NE(buffer2->data, nullptr);
   EXPECT_EQ(buffer2->size_bytes, 123);
 
@@ -161,8 +161,8 @@ TEST(NanoarrowHppTest, NanoarrowHppUniqueBitmapTest) {
 
   // move constructor
   nanoarrow::UniqueBitmap bitmap2 = std::move(bitmap);
-  EXPECT_EQ(bitmap->buffer.data, nullptr);
-  EXPECT_EQ(bitmap->size_bits, 0);
+  EXPECT_EQ(bitmap->buffer.data, nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
+  EXPECT_EQ(bitmap->size_bits, 0);          // NOLINT(clang-analyzer-cplusplus.Move)
   EXPECT_NE(bitmap2->buffer.data, nullptr);
   EXPECT_EQ(bitmap2->size_bits, 123);
 
@@ -250,7 +250,8 @@ TEST(NanoarrowHppTest, NanoarrowHppUniqueArrayViewTest) {
 
   // move constructor
   nanoarrow::UniqueArrayView array_view2 = std::move(array_view);
-  EXPECT_EQ(array_view->storage_type, NANOARROW_TYPE_UNINITIALIZED);
+  EXPECT_EQ(array_view->storage_type,  // NOLINT(clang-analyzer-cplusplus.Move)
+            NANOARROW_TYPE_UNINITIALIZED);
   EXPECT_EQ(array_view2->storage_type, NANOARROW_TYPE_STRUCT);
 
   // pointer constructor

src/nanoarrow/common/schema.c

@@ -1346,7 +1346,7 @@ static inline void ArrowToStringLogChars(char** out, int64_t n_chars_last,
   // In the unlikely snprintf() returning a negative value (encoding error),
   // ensure the result won't cause an out-of-bounds access.
   if (n_chars_last < 0) {
-    n_chars = 0;
+    n_chars_last = 0;
   }
 
   *n_chars += n_chars_last;

src/nanoarrow/common/utils.c

@@ -356,6 +356,7 @@ ArrowErrorCode ArrowDecimalSetDigits(struct ArrowDecimal* decimal,
 // https://github.com/apache/arrow/blob/cd3321b28b0c9703e5d7105d6146c1270bbadd7f/cpp/src/arrow/util/decimal.cc#L365
 ArrowErrorCode ArrowDecimalAppendDigitsToBuffer(const struct ArrowDecimal* decimal,
                                                 struct ArrowBuffer* buffer) {
+  NANOARROW_DCHECK(decimal->n_words == 2 || decimal->n_words == 4);
   int is_negative = ArrowDecimalSign(decimal) < 0;
 
   uint64_t words_little_endian[4];

src/nanoarrow/device/device.c

@@ -492,7 +492,7 @@ static ArrowErrorCode ArrowDeviceArrayViewEnsureBufferSizesAsync(
   NANOARROW_DCHECK(cursor == (buffer.data + buffer.size_bytes));
   ArrowBufferReset(&buffer);
 
-  return NANOARROW_OK;
+  return result;
 }
 
 ArrowErrorCode ArrowDeviceArrayViewSetArrayAsync(

src/nanoarrow/device/device_hpp_test.cc

@@ -27,7 +27,7 @@ TEST(NanoarrowDeviceHpp, UniqueDeviceArray) {
   ASSERT_NE(array->array.release, nullptr);
 
   nanoarrow::device::UniqueDeviceArray array2 = std::move(array);
-  ASSERT_EQ(array->array.release, nullptr);
+  ASSERT_EQ(array->array.release, nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
   ASSERT_NE(array2->array.release, nullptr);
 }
 
@@ -46,7 +46,7 @@ TEST(NanoarrowDeviceHpp, UniqueDeviceArrayStream) {
   ASSERT_NE(stream->release, nullptr);
 
   nanoarrow::device::UniqueDeviceArrayStream stream2 = std::move(stream);
-  ASSERT_EQ(stream->release, nullptr);
+  ASSERT_EQ(stream->release, nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
   ASSERT_NE(stream2->release, nullptr);
 }
 
@@ -57,7 +57,7 @@ TEST(NanoarrowDeviceHpp, UniqueDevice) {
   ArrowDeviceInitCpu(device.get());
 
   nanoarrow::device::UniqueDevice device2 = std::move(device);
-  ASSERT_EQ(device->release, nullptr);
+  ASSERT_EQ(device->release, nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
   ASSERT_NE(device2->release, nullptr);
 }
 
@@ -71,5 +71,6 @@ TEST(NanoarrowDeviceHpp, UniqueDeviceArrayView) {
 
   nanoarrow::device::UniqueDeviceArrayView array_view2 = std::move(array_view);
   ASSERT_EQ(array_view2->array_view.storage_type, NANOARROW_TYPE_INT32);
-  ASSERT_EQ(array_view->array_view.storage_type, NANOARROW_TYPE_UNINITIALIZED);
+  ASSERT_EQ(array_view->array_view.storage_type,  // NOLINT(clang-analyzer-cplusplus.Move)
+            NANOARROW_TYPE_UNINITIALIZED);
 }

src/nanoarrow/device/metal_test.cc

@@ -75,7 +75,6 @@ TEST(NanoarrowDeviceMetal, DeviceGpuBufferMove) {
   struct ArrowBufferView view = {data, sizeof(data)};
 
   ASSERT_EQ(ArrowDeviceBufferInit(cpu, view, gpu, &buffer), NANOARROW_OK);
-  auto mtl_buffer = reinterpret_cast<MTL::Buffer*>(buffer.data);
 
   // GPU -> GPU: just a move
   uint8_t* old_ptr = buffer.data;

src/nanoarrow/ipc/decoder.c

@@ -1506,6 +1506,9 @@ struct ArrowIpcIntervalMonthDayNano {
 static int ArrowIpcDecoderSwapEndian(struct ArrowIpcBufferSource* src,
                                      struct ArrowBufferView* out_view,
                                      struct ArrowBuffer* dst, struct ArrowError* error) {
+  NANOARROW_DCHECK(out_view->size_bytes > 0);
+  NANOARROW_DCHECK(out_view->data.data != NULL);
+
   // Some buffer data types don't need any endian swapping
   switch (src->data_type) {
     case NANOARROW_TYPE_BOOL:
@@ -1539,6 +1542,7 @@ static int ArrowIpcDecoderSwapEndian(struct ArrowIpcBufferSource* src,
       uint64_t* ptr_dst = (uint64_t*)dst->data;
       uint64_t words[4];
       int n_words = (int)(src->element_size_bits / 64);
+      NANOARROW_DCHECK(n_words == 2 || n_words == 4);
 
       for (int64_t i = 0; i < (dst->size_bytes / n_words / 8); i++) {
         for (int j = 0; j < n_words; j++) {

src/nanoarrow/ipc/ipc_hpp_test.cc

@@ -28,7 +28,7 @@ TEST(NanoarrowIpcHppTest, NanoarrowIpcHppTestUniqueDecoder) {
 
   nanoarrow::ipc::UniqueDecoder decoder2 = std::move(decoder);
   EXPECT_NE(decoder2->private_data, nullptr);
-  EXPECT_EQ(decoder->private_data, nullptr);
+  EXPECT_EQ(decoder->private_data, nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
 }
 
 TEST(NanoarrowIpcHppTest, NanoarrowIpcHppTestUniqueEncoder) {
@@ -40,7 +40,7 @@ TEST(NanoarrowIpcHppTest, NanoarrowIpcHppTestUniqueEncoder) {
 
   nanoarrow::ipc::UniqueEncoder encoder2 = std::move(encoder);
   EXPECT_NE(encoder2->private_data, nullptr);
-  EXPECT_EQ(encoder->private_data, nullptr);
+  EXPECT_EQ(encoder->private_data, nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
 }
 
 TEST(NanoarrowIpcHppTest, NanoarrowIpcHppTestUniqueInputStream) {
@@ -54,7 +54,7 @@ TEST(NanoarrowIpcHppTest, NanoarrowIpcHppTestUniqueInputStream) {
 
   nanoarrow::ipc::UniqueInputStream input2 = std::move(input);
   EXPECT_NE(input2->release, nullptr);
-  EXPECT_EQ(input->release, nullptr);
+  EXPECT_EQ(input->release, nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
 }
 
 TEST(NanoarrowIpcHppTest, NanoarrowIpcHppTestUniqueOutputStream) {
@@ -68,5 +68,5 @@ TEST(NanoarrowIpcHppTest, NanoarrowIpcHppTestUniqueOutputStream) {
 
   nanoarrow::ipc::UniqueOutputStream output2 = std::move(output);
   EXPECT_NE(output2->release, nullptr);
-  EXPECT_EQ(output->release, nullptr);
+  EXPECT_EQ(output->release, nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
 }