Merge branch 'topic/bbannier/cmake-clang-tidy'

.cirrus.yml

@@ -37,12 +37,12 @@ lint_task:
 
   env:
     CCACHE_DIR: /tmp/ccache
-    LD_LIBRARY_PATH: /usr/lib/llvm-18/lib/clang/18/lib/linux/
+    LD_LIBRARY_PATH: /usr/lib/llvm-19/lib/clang/19/lib/linux/
 
   update_git_script:
     - git submodule update --recursive --init
 
-  configure_script:   ./ci/run-ci -b build configure debug --cxx-compiler clang++-18 --clang-format `which clang-format-18` --clang-tidy `which clang-tidy-18`
+  configure_script:   ./ci/run-ci -b build configure debug --cxx-compiler clang++-19 --clang-tidy `which clang-tidy-19`
   build_script:       ./ci/run-ci -b build build
   test_code_script:   ./ci/run-ci -b build test-code
 
@@ -56,7 +56,7 @@ lint_task:
     clang_artifacts:
         path: build/ci
 
-clang18_ubuntu_debug_task:
+clang19_ubuntu_debug_task:
   container:
     dockerfile: ci/Dockerfile
     cpu: 4
@@ -72,12 +72,12 @@ clang18_ubuntu_debug_task:
 
   env:
     CCACHE_DIR: /tmp/ccache
-    LD_LIBRARY_PATH: /usr/lib/llvm-18/lib/clang/18/lib/linux/
+    LD_LIBRARY_PATH: /usr/lib/llvm-19/lib/clang/19/lib/linux/
 
   update_git_script:
     - git submodule update --recursive --init
 
-  configure_script:   ./ci/run-ci -b build configure debug --cxx-compiler clang++-18 --clang-format `which clang-format-18` --clang-tidy `which clang-tidy-18`
+  configure_script:   ./ci/run-ci -b build configure debug --cxx-compiler clang++-19
   build_script:       ./ci/run-ci -b build build
   test_build_script:  ./ci/run-ci -b build test-build
   install_script:     ./ci/run-ci -b build install
@@ -98,7 +98,7 @@ clang18_ubuntu_debug_task:
     clang_artifacts:
         path: build/ci
 
-clang18_lts_ubuntu_release_task:
+clang19_lts_ubuntu_release_task:
   container:
     dockerfile: ci/Dockerfile
     cpu: 4
@@ -118,7 +118,7 @@ clang18_lts_ubuntu_release_task:
   update_git_script:
     - git submodule update --recursive --init
 
-  configure_script:      ./ci/run-ci -b build configure release --cxx-compiler clang++-18 --clang-format `which clang-format-18` --clang-tidy `which clang-tidy-18`
+  configure_script:      ./ci/run-ci -b build configure release --cxx-compiler clang++-19
   build_script:          ./ci/run-ci -b build build
   test_build_script:     ./ci/run-ci -b build test-build
   install_script:        ./ci/run-ci -b build install
@@ -139,7 +139,7 @@ clang18_lts_ubuntu_release_task:
       type: text/xml
       format: junit
 
-clang18_lts_ubuntu_release_static_task:
+clang19_lts_ubuntu_release_static_task:
   container:
     dockerfile: ci/Dockerfile
     cpu: 4
@@ -159,7 +159,7 @@ clang18_lts_ubuntu_release_static_task:
   update_git_script:
     - git submodule update --recursive --init
 
-  configure_script:      ./ci/run-ci -b build configure release --cxx-compiler clang++-18 --clang-format `which clang-format-18` --clang-tidy `which clang-tidy-18` --build-static-libs
+  configure_script:      ./ci/run-ci -b build configure release --cxx-compiler clang++-19
   build_script:          ./ci/run-ci -b build build
   install_script:        ./ci/run-ci -b build install
   cleanup_script:        ./ci/run-ci -b build cleanup

.clang-tidy

@@ -4,6 +4,7 @@
 Checks: '
   bugprone-*,
   -bugprone-easily-swappable-parameters,
+  -bugprone-exception-escape,
   -bugprone-reserved-identifier,
   -bugprone-branch-clone,
   -bugprone-macro-parentheses,

3rdparty/.clang-tidy

@@ -0,0 +1,10 @@
+# Copyright (c) 2020-2023 by the Zeek Project. See LICENSE for details.
+
+---
+# We want to disable clang-tidy checks for all files under this directory, but
+# clang-tidy cannot run with all checks disabled. Instead enable an inexpensive
+# check and configure it so it matches nothing, see
+# https://stackoverflow.com/a/58379342/176922.
+Checks: '-*,misc-definitions-in-headers'
+CheckOptions:
+  - { key: HeaderFileExtensions,          value: "x" }

3rdparty/CMakeLists.txt

@@ -1,3 +1,10 @@
+# If we run clang-tidy as part of CMake we never want to check files in this
+# directory. We specify this in addition to the local `.clang-tidy` in this
+# directory since it is only valid in subdirectories which do not provided
+# their on `.clang-tidy` config.
+set(CMAKE_C_CLANG_TIDY "")
+set(CMAKE_CXX_CLANG_TIDY "")
+
 # Note that most of the subdirectories here don't need to be known
 # to CMake because we directly pick out the pieces where we need
 # them.
@@ -7,7 +14,6 @@ add_subdirectory(doctest)
 
 add_subdirectory(justrx)
 
-
 # Use the configured C compiler as ASM compiler as well. This prevents that we
 # e.g., use a Clang as C compiler, but e.g., GCC as assembler which leads to
 # incompatible flags like `-Weverything` being passed to GCC from e.g.,

CHANGES

@@ -1,3 +1,98 @@
+1.13.0-dev.34 | 2025-01-08 09:39:11 +0100
+
+  * Bump `3rdparty/filesystem` to pick up fixes for clang-tidy-19. (Benjamin Bannier, Corelight)
+
+  * Drop unused clang-format integration in `./ci/run-ci`. (Benjamin Bannier, Corelight)
+
+    This was already unused since we moved clang-format to a pre-commit
+    hook.
+
+  * Move `./ci/run-ci`'s clang-tidy check to CMake. (Benjamin Bannier, Corelight)
+
+    Instead of implementing this ourself instea use CMake with
+    `CMAKE_CXX_CLANG_TIDY`/`CMAKE_C_CLANG_TIDY`.
+
+  * Update CMake version in CI for clang-tidy. (Benjamin Bannier, Corelight)
+
+  * Bump LLVM toolchain used in CI. (Benjamin Bannier, Corelight)
+
+  * Fix up APIs in AST API which were not actually moving. (Benjamin Bannier, Corelight)
+
+    Many functions in the AST API were moving arguments only for them to
+    ultimately be copied; `clang-tidy` diagnosed most of these.
+    This patch cleans up the flagged useless passes by value and `std::move`
+    without effect.
+
+  * Fix up container helpers to ensure moving. (Benjamin Bannier, Corelight)
+
+    It is not immediately clear to me when the previous implementation could
+    move their values over. `clang-tidy` ended up flagging many of these as
+    not moving and forcing copies.
+
+    This patch rewrites the loops in these helpers to enforce iteration with
+    move.
+
+  * Disable clang-tidy on generated lexer/parser files. (Benjamin Bannier, Corelight)
+
+  * Reformat code not recognized as C++ by pre-commit hook. (Benjamin Bannier, Corelight)
+
+  * Remove some unneeded includes. (Benjamin Bannier, Corelight)
+
+  * Parenthesize expression when arithmetic mixes difference precedences. (Benjamin Bannier, Corelight)
+
+    `clang-tidy` suggests adding extra parentheses when mixing e.g.,
+    addition and multiplication. Apply these suggestions (automatically).
+
+  * Consistently pass `std::string_view` by value. (Benjamin Bannier, Corelight)
+
+    `std::string_view` is a type which is designed to be (very) cheap to
+    copy.
+
+  * Remove unneeded copies. (Benjamin Bannier, Corelight)
+
+  * Fix APIs taking `std::string_view` assuming them to be null-terminated. (Benjamin Bannier, Corelight)
+
+    In general `std::string_view` values do not need to be null-terminated,
+    e.g., if they reference subranges in a bigger null-terminated string. We
+    were previously passing `std::string_view` in a few places where we
+    assumed them to be null-terminated, e.g., for passing them on to C APIs
+    expecting null-terminated `const char*`. These worked well enough for us
+    for now, but these APIs were generally unsafe.
+
+    This patch switches to instead passing either `const char*` signalling
+    proper, null-terminated strings, or by passing in a `const std::string&`
+    where it should already be present in the caller.
+
+  * Consistently mark functions and values not exposed `static`. (Benjamin Bannier, Corelight)
+
+  * Locally suppress `clang-tidy` diagnostic. (Benjamin Bannier, Corelight)
+
+    This suppresses a `clang-tidy` lint suggesting to mark our default
+    `main` function as not exported. This function is shipped as part of
+    libhilti and intended to be available as a weak symbol.
+
+  * Prefer `std::max` over ad hoc implementations. (Benjamin Bannier, Corelight)
+
+  * Fix APIs where we assumed a string_view was null-terminated. (Benjamin Bannier, Corelight)
+
+    This either switches to using `const char*` to signify guaranteed null
+    termination, or locally suppress clang-tidy warnings about assuming
+    null-terminated string_views.
+
+  * Bring configuration for `clang-tidy` up-to-date. (Benjamin Bannier, Corelight)
+
+    This patch brings our clang-tidy setup up to date. Much of this was
+    built with `./ci/run-ci` `in mind with extra suppressions hardcoded
+    e.g., in `.clang-tidy.ignore`. With this patch we instead configure
+    `.clang-tidy` files so they can suppress diagnostics from certain
+    directories. We also set up 3rdparty code to work with CMake's
+    `clang-tidy` integration[1].
+
+    We also globally suppress some diagnostics which we have no intention of
+    addressing.
+
+    [^1]: https://cmake.org/cmake/help/latest/prop_tgt/LANG_CLANG_TIDY.html#prop_tgt:%3CLANG%3E_CLANG_TIDY
+
 1.13.0-dev.14 | 2025-01-06 16:29:53 +0100
 
   * Do not require owning strings for `startsWith`. (Benjamin Bannier, Corelight)

VERSION

@@ -1 +1 @@
-1.13.0-dev.14
+1.13.0-dev.34

ci/Dockerfile

@@ -14,7 +14,6 @@ RUN apt-get update \
  && apt-get install -y --no-install-recommends \
     bison \
     ccache \
-    cmake \
     doxygen \
     flex \
     g++ \
@@ -40,10 +39,16 @@ RUN apt-get update \
  # GCC.
  && apt-get install -y --no-install-recommends g++ gcc \
  # LLVM toolchain.
- && echo 'deb http://apt.llvm.org/jammy/ llvm-toolchain-jammy-18 main' >> /etc/apt/sources.list.d/llvm18.list \
- && echo 'deb-src http://apt.llvm.org/jammy/ llvm-toolchain-jammy-18 main' >> /etc/apt/sources.list.d/llvm18.list \
+ && echo 'deb http://apt.llvm.org/jammy/ llvm-toolchain-jammy-19 main' >> /etc/apt/sources.list.d/llvm19.list \
+ && echo 'deb-src http://apt.llvm.org/jammy/ llvm-toolchain-jammy-19 main' >> /etc/apt/sources.list.d/llvm19.list \
  && curl https://apt.llvm.org/llvm-snapshot.gpg.key -o /etc/apt/trusted.gpg.d/llvm.asc \
  && apt-get update \
- && apt-get install -y --no-install-recommends llvm-18-dev clang-18 libclang-18-dev clang-format-18 clang-tidy-18 libclang-rt-18-dev \
+ && apt-get install -y --no-install-recommends llvm-19-dev clang-19 libclang-19-dev clang-tidy-19 libclang-rt-19-dev \
+ # Install a recent CMake for `SKIP_LINTING` support.
+ && test -f /usr/share/doc/kitware-archive-keyring/copyright || \
+    curl https://apt.kitware.com/keys/kitware-archive-latest.asc 2>/dev/null | gpg --dearmor - | tee /usr/share/keyrings/kitware-archive-keyring.gpg >/dev/null \
+ && echo 'deb [signed-by=/usr/share/keyrings/kitware-archive-keyring.gpg] https://apt.kitware.com/ubuntu/ jammy main' | tee /etc/apt/sources.list.d/kitware.list >/dev/null \
+ && apt-get update \
+ && apt-get install -y --no-install-recommends kitware-archive-keyring cmake \
  && apt-get clean \
  && rm -rf /var/lib/apt/lists/*

ci/run-ci

@@ -45,7 +45,6 @@ Global options:
 Configure options:
 
     --build-toolchain={yes,no}     Build the Spicy compiler toolchain [default: yes]
-    --clang-format <path>          Path to clang-format to use (default: found in PATH)
     --clang-tidy <path>            Path to clang-tidy to use   (default: found in PATH)
     --cxx-compiler <path>          Path to C++ compiler to use (default: found by cmake)
     --disable-precompiled-headers  Disable use of precompiled headers for developer tests
@@ -92,7 +91,6 @@ function run_configure {
     mkdir -p ${install}
     configure="./configure --builddir=${build} --prefix=${install} --generator=Ninja --enable-werror --enable-ccache --with-hilti-compiler-launcher=ccache"
 
-    clang_format=$(which clang-format 2>/dev/null)
     clang_tidy=$(which clang-tidy 2>/dev/null)
 
     if [ "${build_type}" == "release" ]; then
@@ -111,13 +109,6 @@ function run_configure {
                 shift 2;
                 ;;
 
-            --clang-format)
-                test $# -gt 0 || usage
-                clang_format="$2"
-                test -x ${clang_format} || error "clang-format not found in $2"
-                shift 2;
-                ;;
-
             --clang-tidy)
                 test $# -gt 0 || usage
                 clang_tidy="$2"
@@ -153,7 +144,6 @@ function run_configure {
         error "Build directory ${build} already exists, delete first"
     fi
 
-    test -z "${clang_format}" && log_stage "Warning: No clang-format found, will skip any related tests"
     test -z "${clang_tidy}" && log_stage "Warning: No clang-tidy found, will skip any related tests"
 
     # Looks like Cirrus CI doesn't fetch tags.
@@ -164,8 +154,13 @@ function run_configure {
     ${configure} || exit 1
     mkdir -p ${artifacts}
 
-    echo "${clang_format}" >${build}/.clang_format
     echo "${clang_tidy}" >${build}/.clang_tidy
+
+    if [ -x "${clang_tidy}" ]; then
+        pushd "${build}" >/dev/null || exit 1
+        cmake -DCMAKE_CXX_CLANG_TIDY="${clang_tidy}" -DCMAKE_C_CLANG_TIDY="${clang_tidy}" ..
+        popd >/dev/null || exit 1
+    fi
 }
 
 function run_build {
@@ -233,38 +228,11 @@ function run_test_btest {
     execute_btest "Spicy" "tests" "${alternative}"
 }
 
-function run_test_clang_tidy {
-    clang_tidy=$(cat ${build}/.clang_tidy 2>/dev/null)
-
-    if [ ! -x ${clang_tidy} ]; then
-        log_warning "clang-tidy not available, skipping"
-        return 0
-    fi
-
-    log_stage "Running clang-tidy ..."
-
-    export CLANG_TIDY=${clang_tidy}
-
-    if ${root}/scripts/run-clang-tidy --fixit -j 3 ${build}; then
-        echo "clang-tidy run passed"
-        return 0
-    else
-        git diff | tee ${artifacts}/clang-tidy.diff
-        log_diag "Patch to what can be fixed automatically in 'clang-tidy.diff'"
-        log_error "clang-tidy has failed"
-        return 1
-    fi
-}
-
 function run_test_code {
     rc=0
 
     pre-commit run -a --show-diff-on-failure || rc=1
 
-    pushd $(pwd) >/dev/null
-    run_test_clang_tidy || rc=1
-    popd >/dev/null
-
     return ${rc}
 }
 

hilti/runtime/include/3rdparty/.clang-tidy

@@ -0,0 +1 @@
+../../../../3rdparty/.clang-tidy