Address review comments

Author: mordante

libcxx/CMakeLists.txt

@@ -124,10 +124,6 @@ option(LIBCXX_ENABLE_VENDOR_AVAILABILITY_ANNOTATIONS
    the shared library they shipped should turn this on and see `include/__availability`
    for more details." OFF)
 option(LIBCXX_ENABLE_CLANG_TIDY "Whether to compile and run clang-tidy checks" OFF)
-set(LIBCXX_CLANG_TIDY_EXECUTABLE "" CACHE FILEPATH
-  "The clang-tidy executable to use to run the clang-tidy checks.
-   When the variable is not set cmake will search for clang-tidy.
-   This is only used when LIBCXX_ENABLE_CLANG_TIDY is ON.")
 
 if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
   set(LIBCXX_DEFAULT_TEST_CONFIG "llvm-libc++-shared-gcc.cfg.in")

libcxx/test/configs/cmake-bridge.cfg.in

@@ -25,7 +25,6 @@ config.test_exec_root = os.path.join('@CMAKE_BINARY_DIR@', 'test')
 # Add substitutions for bootstrapping the test suite configuration
 import shlex
 config.substitutions.append(('%{cxx}', shlex.quote('@CMAKE_CXX_COMPILER@')))
-config.substitutions.append(('%{clang-tidy}', shlex.quote('@LIBCXX_CLANG_TIDY@')))
 config.substitutions.append(('%{libcxx-dir}', '@LIBCXX_SOURCE_DIR@'))
 config.substitutions.append(('%{include-dir}', '@LIBCXX_GENERATED_INCLUDE_DIR@'))
 config.substitutions.append(('%{target-include-dir}', '@LIBCXX_GENERATED_INCLUDE_TARGET_DIR@'))

libcxx/test/tools/CMakeLists.txt

@@ -3,5 +3,10 @@ set(LIBCXX_TEST_TOOLS_PATH ${CMAKE_CURRENT_BINARY_DIR} PARENT_SCOPE)
 
 # TODO: Remove LIBCXX_ENABLE_CLANG_TIDY
 if(LIBCXX_ENABLE_CLANG_TIDY)
+  if(NOT CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+    message(STATUS "Clang-tidy can only be used when building libc++ with "
+                   "a clang compiler.")
+    return()
+  endif()
   add_subdirectory(clang_tidy_checks)
 endif()

libcxx/test/tools/clang_tidy_checks/CMakeLists.txt

@@ -5,81 +5,10 @@
 set(LLVM_DIR_SAVE ${LLVM_DIR})
 set(Clang_DIR_SAVE ${Clang_DIR})
 
-# libc++ only supports the latest two released versions of clang-tidy. During
-# the release period there the new unreleased version is also supported. To keep
-# this code simple, always accept the last 3 versions.
-set(HEAD ${LLVM_VERSION_MAJOR})
-math(EXPR MINUS_1 "${HEAD} - 1")
-math(EXPR MINUS_2 "${MINUS_1} - 1")
-math(EXPR MINUS_3 "${MINUS_2} - 1")
-
-if("${LIBCXX_CLANG_TIDY_EXECUTABLE}" STREQUAL "")
-  set(NAMES "clang-tidy-${HEAD}"
-            "clang-tidy-${MINUS_1}"
-            "clang-tidy-${MINUS_2}"
-            "clang-tidy-${MINUS_3}"
-            "clang-tidy")
-  if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
-    string(REGEX MATCH
-           "[0-9]+"
-           LIBCXX_CLANG_MAJOR
-           "${CMAKE_CXX_COMPILER_VERSION}")
-    set(NAMES "clang-tidy-${LIBCXX_CLANG_MAJOR}"
-              "clang-tidy")
-  endif()
-  find_program(LIBCXX_CLANG_TIDY NAMES ${NAMES})
-else()
-  set(LIBCXX_CLANG_TIDY "${LIBCXX_CLANG_TIDY_EXECUTABLE}" CACHE INTERNAL "")
-endif()
-
-if("${LIBCXX_CLANG_TIDY}" STREQUAL "")
-  set(LIBCXX_CLANG_TIDY "" CACHE INTERNAL "")
-  message(STATUS "Could not find a clang-tidy executable;
-                  custom libc++ clang-tidy checks will not be available.")
-  return()
-endif()
-
-execute_process(COMMAND
-            "${LIBCXX_CLANG_TIDY}" "--version"
-            OUTPUT_VARIABLE CLANG_TIDY_VERSION_OUTPUT
-            COMMAND_ERROR_IS_FATAL ANY)
-string(REGEX MATCH
-       "[0-9]+\.[0-9]+\.[0-9]+"
-       LIBCXX_CLANG_TIDY_VERSION
-       "${CLANG_TIDY_VERSION_OUTPUT}")
-
-if("${LIBCXX_CLANG_TIDY_VERSION}" VERSION_LESS "${MINUS_3}")
-  set(LIBCXX_CLANG_TIDY "" CACHE INTERNAL "")
-  message(STATUS "The clang-tidy version found ${LIBCXX_CLANG_TIDY_VERSION} is
-                  too old, version ${MINUS_3}.0.0 is required;
-                  custom libc++ clang-tidy checks will not be available.")
-  return()
-endif()
-
-# For modules clang-tidy 17 or newer is required. This is due to the state of
-# implementation in clang/clang-tidy. This additional restriction should be
-# temporary.
-# TODO(LLVM-19) Remove this check
-if("${LIBCXX_CLANG_TIDY_VERSION}" VERSION_LESS "17.0.0")
-  message(STATUS "The clang-tidy version found ${LIBCXX_CLANG_TIDY_VERSION} is
-                  too old, version 17.0.0 is required;
-                  custom libc++ clang-tidy checks will not be available.")
-endif()
-
-if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang" AND
-   NOT CMAKE_CXX_COMPILER_VERSION VERSION_EQUAL "${LIBCXX_CLANG_TIDY_VERSION}")
-
-  set(LIBCXX_CLANG_TIDY "" CACHE INTERNAL "")
-  message(STATUS "The version of clang ${CMAKE_CXX_COMPILER_VERSION} differs
-                  from the version of clang-tidy ${LIBCXX_CLANG_TIDY_VERSION};
-                  custom libc++ clang-tidy checks will not be available.")
-  return()
-endif()
-
 # Since the Clang C++ ABI is not stable the Clang libraries and clang-tidy
 # version must match. Otherwise there likely will be ODR-violations. This had
 # led to crashes and incorrect output of the clang-tidy based checks.
-find_package(Clang "${LIBCXX_CLANG_TIDY_VERSION}")
+find_package(Clang ${CMAKE_CXX_COMPILER_VERSION})
 
 set(SOURCES
     abi_tag_on_virtual.cpp
@@ -94,7 +23,6 @@ set(SOURCES
    )
 
 if(NOT Clang_FOUND)
-  set(LIBCXX_CLANG_TIDY "" CACHE INTERNAL "")
   message(STATUS "Could not find a suitable version of the Clang development package;
                   custom libc++ clang-tidy checks will not be available.")
   return()

libcxx/utils/ci/run-buildbot

@@ -47,10 +47,6 @@ CLANG_FORMAT        The clang-format binary to use when generating the format
 ENABLE_CLANG_TIDY   Whether to compile and run clang-tidy checks. This variable
                     is optional.
 
-CLANG_TIDY_EXECUTABLE
-                    The clang-tidy executable to use. Its version should match
-                    the clang version used. This variable is optional.
-
 EOF
 }
 
@@ -119,10 +115,6 @@ if [ -z "${ENABLE_CLANG_TIDY}" ]; then
     ENABLE_CLANG_TIDY=Off
 fi
 
-if [ -z "${CLANG_TIDY_EXECUTABLE}" ]; then
-   CLANG_TIDY_EXECUTABLE=''
-fi
-
 function generate-cmake-base() {
     echo "--- Generating CMake"
     ${CMAKE} \
@@ -135,7 +127,6 @@ function generate-cmake-base() {
           -DLIBCXXABI_ENABLE_WERROR=YES \
           -DLIBUNWIND_ENABLE_WERROR=YES \
           -DLIBCXX_ENABLE_CLANG_TIDY=${ENABLE_CLANG_TIDY} \
-		  -DLIBCXX_CLANG_TIDY_EXECUTABLE=${CLANG_TIDY_EXECUTABLE} \
           -DLLVM_LIT_ARGS="-sv --xunit-xml-output test-results.xml --timeout=1500 --time-tests" \
           "${@}"
 }

libcxx/utils/libcxx/test/config.py

@@ -9,13 +9,6 @@
 import os
 
 
-def _hasSubstitution(substitution, config):
-    for k, v in config.substitutions:
-        if k == substitution:
-            return True
-    return False
-
-
 def _getSubstitution(substitution, config):
     for (orig, replacement) in config.substitutions:
         if orig == substitution:

libcxx/utils/libcxx/test/features.py

@@ -7,7 +7,6 @@
 # ===----------------------------------------------------------------------===##
 
 from libcxx.test.dsl import *
-import libcxx.test.config as config
 from lit.BooleanExpression import BooleanExpression
 import re
 import shutil
@@ -24,7 +23,6 @@
 _isMSVC = lambda cfg: "_MSC_VER" in compilerMacros(cfg)
 _msvcVersion = lambda cfg: (int(compilerMacros(cfg)["_MSC_VER"]) // 100, int(compilerMacros(cfg)["_MSC_VER"]) % 100)
 
-
 def _getAndroidDeviceApi(cfg):
     return int(
         programOutput(
@@ -275,11 +273,6 @@ def _getAndroidDeviceApi(cfg):
         name="executor-has-no-bash",
         when=lambda cfg: runScriptExitCode(cfg, ["%{exec} bash -c 'bash --version'"]) != 0,
     ),
-    Feature(
-        name="has-clang-tidy",
-        when=lambda cfg: config._hasSubstitution("%{clang-tidy}", cfg)
-        and config._getSubstitution("%{clang-tidy}", cfg) != "",
-    ),
     # Whether module support for the platform is available.
     Feature(
         name="has-no-cxx-module-support",

libcxx/utils/libcxx/test/params.py

@@ -53,17 +53,14 @@
     "-Wunused-parameter",
     "-Wunreachable-code",
     "-Wno-unused-local-typedef",
-
     # Disable warnings for extensions used in C++03
     "-Wno-local-type-template-args",
     "-Wno-c++11-extensions",
-
     # TODO(philnik) This fails with the PSTL.
     "-Wno-unknown-pragmas",
     # Don't fail compilation in case the compiler fails to perform the requested
     # loop vectorization.
     "-Wno-pass-failed",
-
     # TODO: Find out why GCC warns in lots of places (is this a problem with always_inline?)
     "-Wno-dangling-reference",
     "-Wno-mismatched-new-delete",
@@ -110,6 +107,37 @@ def getSizeOptimizationFlag(cfg):
         )
 
 
+def testClangTidy(cfg, version, executable):
+    try:
+        if version in commandOutput(cfg, [f"{executable} --version"]):
+            return executable
+    except ConfigurationRuntimeError:
+        return None
+
+
+def getSuitableClangTidy(cfg):
+    # If we didn't build the libcxx-tidy plugin via CMake, we can't run the clang-tidy tests.
+    if (
+        runScriptExitCode(
+            cfg, ["stat %{test-tools-dir}/clang_tidy_checks/libcxx-tidy.plugin"]
+        )
+        != 0
+    ):
+        return None
+
+    version = "{__clang_major__}.{__clang_minor__}.{__clang_patchlevel__}".format(
+        **compilerMacros(cfg)
+    )
+    exe = testClangTidy(
+        cfg, version, "clang-tidy-{__clang_major__}".format(**compilerMacros(cfg))
+    )
+
+    if not exe:
+        exe = testClangTidy(cfg, version, "clang-tidy")
+
+    return exe
+
+
 # fmt: off
 DEFAULT_PARAMETERS = [
     Parameter(
@@ -366,6 +394,16 @@ def getSizeOptimizationFlag(cfg):
         default=f"{shlex.quote(sys.executable)} {shlex.quote(str(Path(__file__).resolve().parent.parent.parent / 'run.py'))}",
         help="Custom executor to use instead of the configured default.",
         actions=lambda executor: [AddSubstitution("%{executor}", executor)],
-    )
+    ),
+    Parameter(
+        name='clang-tidy-executable',
+        type=str,
+        default=lambda cfg: getSuitableClangTidy(cfg),
+        help="TODO",
+        actions=lambda exe: [] if exe is None else [
+            AddFeature('has-clang-tidy'),
+            AddSubstitution('%{clang-tidy}', exe),
+        ]
+     ),
 ]
 # fmt: on