build(clang-tidy): replace clang-tidy scripts with CMake approach (#142)

This PR replaces the clang-tidy scripts with a CMake-based approach. The
core mechanism is to define CMake's `CMAKE_CXX_CLANG_TIDY` variable,
which invokes clang-tidy whenever a C++ target is compiled. The
remainder consists of some logic that finds the clang-tidy executable
and its arguments as well as activating and deactivating the command in
the right place (in particular, to exclude checking third-party code).

I see three main advantages to the new approach: (1) The new logic is
considerably simpler than the previous scripts. (2) The code is
automatically checked when it compiled, potentially saving round-trips
through CI. (3) We save an extra 10 minutes on CI for recompiling the
whole project, which the previous scripts did.

The PR also does two minor changes: (1) It removes the
`AnalyzeTemporaryDtors` configuration, which was depcrecated in clang 16
and removed in clang 18. (2) It applies the suggested fixes for one
class of errors that my clang-tidy-22 complained about.

Signed-off-by: Ingo Müller <ingomueller@google.com>

Author: ingomueller-net

.clang-tidy

@@ -78,7 +78,6 @@ Checks: >
   -bugprone-unchecked-optional-access
 WarningsAsErrors: '*'
 HeaderFilterRegex: 'io/substrait/*.\\.h$'
-AnalyzeTemporaryDtors: false
 FormatStyle: none
 CheckOptions:
   - { key: readability-identifier-naming.NamespaceCase,                            value: lower_case}

.github/workflows/build_test.yml

@@ -25,8 +25,6 @@ jobs:
         run: ./scripts/run-clang-format.sh
       - name: Checking formatting
         run: git diff --exit-code
-      - name: Checking code style
-        run: ./scripts/run-clang-tidy.sh
 
   ubuntu-build-and-test:
     runs-on: ubuntu-latest

CMakeLists.txt

@@ -56,6 +56,11 @@ set(ANTLR_EXECUTABLE "${ANTLR_EXECUTABLE_DIR}/antlr.jar")
 include_directories(include)
 include_directories(src)
 
+include(cmake/clang-tidy.cmake)
+
+# Disable clang-tidy for third-party dependencies.
+unset(CMAKE_CXX_CLANG_TIDY)
+
 # TODO: Simplify once we can require cmake 3.27 (where CONFIG is default).
 
 # Due to packaging changes we use the combined protobuf/absl packaging if
@@ -78,6 +83,7 @@ else()
 endif()
 
 add_subdirectory(third_party)
+set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_COMMAND}) # Re-enable clang-tidy.
 
 list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake_modules")
 include(BuildUtils)

Makefile

@@ -27,16 +27,6 @@ format:
 	bash scripts/run-cmake-format.sh &&  bash scripts/run-clang-format.sh
 	set +f
 
-tidy:
-	set -f
-	bash scripts/run-clang-tidy.sh
-	set +f
-
-tidy-fix:
-	set -f
-	bash scripts/run-clang-tidy.sh fix
-	set +f
-
 debug:
 	@$(MAKE) build-common BUILD_TYPE=Debug
 	@$(MAKE) build BUILD_TYPE=Debug

README.md

@@ -31,10 +31,13 @@ Basically the coding style is based on Google C++ Style, but there are some nami
 - Variable case style change to 'camelBack'
 - Class Member case style change to 'camelBack' with '_' as suffix
 
-For more detail information please refer to .clang-tidy under root directory.
+For more detail information please refer to [`.clang-tidy`](.clang-tidy).
 
+You can run `make format` script to formatting source code. To enable `clang-tidy` checks during compilation, use the options from [`cmake/clang-tidy.cmake`](cmake/clang-tidy.cmake). To apply the suggested fixes, run the following command in the root directory:
 
-You can run `make format` script to formatting source code and run `make tidy` to checking coding style, and run `make tidy-fix`to fix the coding style automatically.
+```bash
+clang-apply-replacements-22 --ignore-insert-conflict build/clang-tidy-fixes/
+```
 
 ## License
 

cmake/clang-tidy.cmake

@@ -0,0 +1,31 @@
+# SPDX-License-Identifier: Apache-2.0
+
+option(SUBSTRAIT_CPP_ENABLE_CLANG_TIDY
+       "Enable clang-tidy static analysis on Substrait MLIR targets" OFF)
+option(SUBSTRAIT_CPP_CLANG_TIDY_PATH "path to 'clang-tidy' executable")
+
+if(SUBSTRAIT_CPP_ENABLE_CLANG_TIDY)
+  # Find `clang-tidy` executable.
+  get_filename_component(CLANG_TIDY_NAME "${SUBSTRAIT_CPP_CLANG_TIDY_PATH}"
+                         NAME)
+  get_filename_component(CLANG_TIDY_DIR "${SUBSTRAIT_CPP_CLANG_TIDY_PATH}"
+                         DIRECTORY)
+  find_program(
+    CLANG_TIDY_EXE
+    HINTS ${CLANG_TIDY_DIR}
+    NAMES "clang-tidy" "${CLANG_TIDY_NAME}" REQUIRED)
+  if("${CLANG_TIDY_EXE}" STREQUAL "")
+    message(FATAL_ERROR "failed to find clang-tidy executable")
+  endif()
+
+  # Assemble command line.
+  set(CLANG_TIDY_CONFIG_FILE "${CMAKE_SOURCE_DIR}/.clang-tidy")
+  set(CLANG_TIDY_COMMAND
+      "${CLANG_TIDY_EXE};--config-file=${CLANG_TIDY_CONFIG_FILE}")
+  set(CMAKE_CXX_CLANG_TIDY_EXPORT_FIXES_DIR
+      "${CMAKE_BINARY_DIR}/clang-tidy-fixes")
+  message(STATUS "clang-tidy enabled, command: ${CLANG_TIDY_COMMAND}")
+endif(SUBSTRAIT_CPP_ENABLE_CLANG_TIDY)
+
+# Enable clang-tidy for all C++ targets.
+set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_COMMAND})

scripts/run-clang-tidy.py

@@ -75,7 +75,9 @@ foreach(PROTO_FILE IN LISTS PROTOBUF_FILELIST)
 endforeach()
 
 # Add the generated protobuf C++ files to our exported library.
+unset(CMAKE_CXX_CLANG_TIDY)
 add_library(substrait_proto ${PROTO_SRCS} ${PROTO_HDRS} ProtoUtils.cpp)
+set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_COMMAND}) # Re-enable clang-tidy.
 target_sources(
   substrait_proto
   PUBLIC FILE_SET

scripts/run-clang-tidy.sh

@@ -12,17 +12,17 @@ std::ostream& operator<<(std::ostream& os, const ParseResult& result) {
   }
   auto msgs = result.getSyntaxErrors();
   if (!msgs.empty()) {
-    os << "{" << std::endl;
+    os << "{" << '\n';
     for (const std::string& msg : msgs) {
-      os << "  \"" << msg << "\"," << std::endl;
+      os << "  \"" << msg << "\"," << '\n';
     }
     os << "}";
   }
   msgs = result.getSemanticErrors();
   if (!msgs.empty()) {
-    os << "{" << std::endl;
+    os << "{" << '\n';
     for (const std::string& msg : msgs) {
-      os << "  \"" << msg << "\"," << std::endl;
+      os << "  \"" << msg << "\"," << '\n';
     }
     os << "}";
   }