Run clang-tidy checks in PR CI (#17407)

We discussed clang-tidy during the cuDF brown bag session. We decided to enable clang-tidy in PR CI and follow up by enabling more checks later.

Authors:
  - Bradley Dice (https://github.com/bdice)
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - David Wendt (https://github.com/davidwendt)

URL: #17407

.github/workflows/pr.yaml

@@ -16,6 +16,7 @@ jobs:
       - changed-files
       - checks
       - conda-cpp-build
+      - cpp-linters
       - conda-cpp-checks
       - conda-cpp-tests
       - conda-python-build
@@ -113,6 +114,13 @@ jobs:
     uses: rapidsai/shared-workflows/.github/workflows/[email protected]
     with:
       build_type: pull-request
+  cpp-linters:
+    secrets: inherit
+    needs: checks
+    uses: rapidsai/shared-workflows/.github/workflows/[email protected]
+    with:
+      build_type: pull-request
+      run_script: "ci/cpp_linters.sh"
   conda-cpp-checks:
     needs: conda-cpp-build
     secrets: inherit

ci/cpp_linters.sh

@@ -25,7 +25,12 @@ RAPIDS_VERSION_MAJOR_MINOR="$(rapids-version-major-minor)"
 source rapids-configure-sccache
 
 # Run the build via CMake, which will run clang-tidy when CUDF_STATIC_LINTERS is enabled.
-cmake -S cpp -B cpp/build -DCMAKE_BUILD_TYPE=Release -DCUDF_STATIC_LINTERS=ON -GNinja
+
+iwyu_flag=""
+if [[ "${RAPIDS_BUILD_TYPE}" == "nightly" ]]; then
+  iwyu_flag="-DCUDF_IWYU=ON"
+fi
+cmake -S cpp -B cpp/build -DCMAKE_BUILD_TYPE=Release -DCUDF_CLANG_TIDY=ON ${iwyu_flag} -DBUILD_TESTS=OFF -GNinja
 cmake --build cpp/build 2>&1 | python cpp/scripts/parse_iwyu_output.py
 
 # Remove invalid components of the path for local usage. The path below is

cpp/.clang-tidy

@@ -39,7 +39,7 @@ Checks:
        -clang-analyzer-optin.core.EnumCastOutOfRange,
        -clang-analyzer-optin.cplusplus.UninitializedObject'
 
-WarningsAsErrors: ''
+WarningsAsErrors: '*'
 HeaderFilterRegex: '.*cudf/cpp/(src|include).*'
 ExcludeHeaderFilterRegex: '.*(Message_generated.h|Schema_generated.h|brotli_dict.hpp|unbz2.hpp|cxxopts.hpp).*'
 FormatStyle:     none

cpp/CMakeLists.txt

@@ -91,7 +91,8 @@ option(
   ${DEFAULT_CUDF_BUILD_STREAMS_TEST_UTIL}
 )
 mark_as_advanced(CUDF_BUILD_STREAMS_TEST_UTIL)
-option(CUDF_STATIC_LINTERS "Enable static linters during compilation" OFF)
+option(CUDF_CLANG_TIDY "Enable clang-tidy during compilation" OFF)
+option(CUDF_IWYU "Enable IWYU during compilation" OFF)
 
 option(
   CUDF_KVIKIO_REMOTE_IO
@@ -159,9 +160,7 @@ endif()
 
 # ##################################################################################################
 # * linter configuration ---------------------------------------------------------------------------
-if(CUDF_STATIC_LINTERS)
-  # For simplicity, for now we assume that all linters can be installed into an environment where
-  # any linter is being run. We could relax this requirement if desired.
+if(CUDF_CLANG_TIDY)
   find_program(
     CLANG_TIDY_EXE
     NAMES "clang-tidy"
@@ -188,7 +187,9 @@ if(CUDF_STATIC_LINTERS)
         "clang-tidy version ${expected_clang_tidy_version} is required, but found ${LLVM_VERSION}"
     )
   endif()
+endif()
 
+if(CUDF_IWYU)
   find_program(IWYU_EXE NAMES include-what-you-use iwyu REQUIRED)
 endif()
 
@@ -201,38 +202,36 @@ function(enable_static_checkers target)
     _LINT "${_tidy_options}" "${_tidy_one_value}" "${_tidy_multi_value}" ${ARGN}
   )
 
-  if(CUDF_STATIC_LINTERS)
-    if(_LINT_CLANG_TIDY)
-      # clang will complain about unused link libraries on the compile line unless we specify
-      # -Qunused-arguments.
-      set_target_properties(
-        ${target} PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_EXE};--extra-arg=-Qunused-arguments"
-      )
-    endif()
-    if(_LINT_IWYU)
-      # A few extra warnings pop up when building with IWYU. I'm not sure why, but they are not
-      # relevant since they don't show up in any other build so it's better to suppress them until
-      # we can figure out the cause. Setting this as part of CXX_INCLUDE_WHAT_YOU_USE does not
-      # appear to be sufficient, we must also ensure that it is set to the underlying target's CXX
-      # compile flags. To do this completely cleanly we should modify the flags on the target rather
-      # than the global CUDF_CXX_FLAGS, but this solution is good enough for now since we never run
-      # the linters on real builds.
-      foreach(_flag -Wno-missing-braces -Wno-unneeded-internal-declaration)
-        list(FIND CUDF_CXX_FLAGS "${_flag}" _flag_index)
-        if(_flag_index EQUAL -1)
-          list(APPEND CUDF_CXX_FLAGS ${_flag})
-        endif()
-      endforeach()
-      set(CUDF_CXX_FLAGS
-          "${CUDF_CXX_FLAGS}"
-          PARENT_SCOPE
-      )
-      set_target_properties(${target} PROPERTIES CXX_INCLUDE_WHAT_YOU_USE "${IWYU_EXE}")
-    endif()
-    foreach(file IN LISTS _LINT_SKIPPED_FILES)
-      set_source_files_properties(${file} PROPERTIES SKIP_LINTING ON)
+  if(_LINT_CLANG_TIDY)
+    # clang will complain about unused link libraries on the compile line unless we specify
+    # -Qunused-arguments.
+    set_target_properties(
+      ${target} PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_EXE};--extra-arg=-Qunused-arguments"
+    )
+  endif()
+  if(_LINT_IWYU)
+    # A few extra warnings pop up when building with IWYU. I'm not sure why, but they are not
+    # relevant since they don't show up in any other build so it's better to suppress them until we
+    # can figure out the cause. Setting this as part of CXX_INCLUDE_WHAT_YOU_USE does not appear to
+    # be sufficient, we must also ensure that it is set to the underlying target's CXX compile
+    # flags. To do this completely cleanly we should modify the flags on the target rather than the
+    # global CUDF_CXX_FLAGS, but this solution is good enough for now since we never run the linters
+    # on real builds.
+    foreach(_flag -Wno-missing-braces -Wno-unneeded-internal-declaration)
+      list(FIND CUDF_CXX_FLAGS "${_flag}" _flag_index)
+      if(_flag_index EQUAL -1)
+        list(APPEND CUDF_CXX_FLAGS ${_flag})
+      endif()
     endforeach()
+    set(CUDF_CXX_FLAGS
+        "${CUDF_CXX_FLAGS}"
+        PARENT_SCOPE
+    )
+    set_target_properties(${target} PROPERTIES CXX_INCLUDE_WHAT_YOU_USE "${IWYU_EXE}")
   endif()
+  foreach(file IN LISTS _LINT_SKIPPED_FILES)
+    set_source_files_properties(${file} PROPERTIES SKIP_LINTING ON)
+  endforeach()
 endfunction()
 
 # ##################################################################################################
@@ -812,9 +811,18 @@ set_target_properties(
 
 # Note: This must come before the target_compile_options below so that the function can modify the
 # flags if necessary.
-enable_static_checkers(
-  cudf SKIPPED_FILES src/io/comp/cpu_unbz2.cpp src/io/comp/brotli_dict.cpp CLANG_TIDY IWYU
-)
+if(CUDF_CLANG_TIDY OR CUDF_IWYU)
+  set(linters)
+  if(CUDF_CLANG_TIDY)
+    list(APPEND linters CLANG_TIDY)
+  endif()
+  if(CUDF_IWYU)
+    list(APPEND linters IWYU)
+  endif()
+  enable_static_checkers(
+    cudf SKIPPED_FILES src/io/comp/cpu_unbz2.cpp src/io/comp/brotli_dict.cpp ${linters}
+  )
+endif()
 target_compile_options(
   cudf PRIVATE "$<$<COMPILE_LANGUAGE:CXX>:${CUDF_CXX_FLAGS}>"
                "$<$<COMPILE_LANGUAGE:CUDA>:${CUDF_CUDA_FLAGS}>"

cpp/include/cudf/ast/expressions.hpp

@@ -612,19 +612,19 @@ class tree {
    * @brief get the first expression in the tree
    * @returns the first inserted expression into the tree
    */
-  expression const& front() const { return *expressions.front(); }
+  [[nodiscard]] expression const& front() const { return *expressions.front(); }
 
   /**
    * @brief get the last expression in the tree
    * @returns the last inserted expression into the tree
    */
-  expression const& back() const { return *expressions.back(); }
+  [[nodiscard]] expression const& back() const { return *expressions.back(); }
 
   /**
    * @brief get the number of expressions added to the tree
    * @returns the number of expressions added to the tree
    */
-  size_t size() const { return expressions.size(); }
+  [[nodiscard]] size_t size() const { return expressions.size(); }
 
   /**
    * @brief get the expression at an index in the tree. Index is checked.

cpp/src/io/json/parser_features.cpp

@@ -109,7 +109,7 @@ struct allnull_column_functor {
   rmm::device_async_resource_ref mr;
 
  private:
-  auto make_zeroed_offsets(size_type size) const
+  [[nodiscard]] auto make_zeroed_offsets(size_type size) const
   {
     auto offsets_buff =
       cudf::detail::make_zeroed_device_uvector_async<size_type>(size + 1, stream, mr);