[libcxx-commits] [PATCH] D131963: [libc++] Add custom clang-tidy checks

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I think we have to options here:

1. Use the clang-tidy in the LLVM monorepo.
2. Use a system-provided clang-tidy if there is one, and disable these tests otherwise.

I think (1) is a non-starter, because we can't start depending on clang-tidy (which depends on clang, LLVM, etc.) inside libc++. That would require doing a bootstrapping build every time we want to run the tests.

So I think we have to to (2), and simply make sure that our Docker image in the CI does have the development packages for clang-tidy.



================
Comment at: libcxx/CMakeLists.txt:316-318
+option(LIBCXX_ENABLE_CUSTOM_CLANG_TIDY_CHECKS OFF "Compile custom clang-tidy checks")
+
+message(STATUS "Target file name: ${LIBCXX_CLANG_TIDY_PATH}")
----------------
Let's remove this and make this automatic based on whether we have the dev packages. Basically, I don't see a reason to disable this if we can have them enabled.


================
Comment at: libcxx/test/tools/clang_tidy_checks/CMakeLists.txt:5
+set(SOURCES
+    robust_against_adl.cpp
+    libcpp_module.cpp)
----------------
Does this render our other tests for robustness against ADL obsolete? If so, we should take one of those, make it regress, and confirm that this clang-tidy check will catch it. If it doesn't, we should understand why.

Basically, I'd like us to have a clear understanding of whether we need to write manual tests for ADL robustness or not.


================
Comment at: libcxx/test/tools/clang_tidy_checks/CMakeLists.txt:9
+
+if(${Clang_FOUND} AND ${LLVM_VERSION_MAJOR} GREATER_EQUAL 15)
+  message(STATUS "Found LLVM ${LLVM_PACKAGE_VERSION}")
----------------
Instead, let's do

```
if(NOT Clang_FOUND OR ${LLVM_VERSION_MAJOR} LESS 15)
  message(STATUS "Could not find a suitable version of the Clang development package, custom libc++ clang-tidy checks will not be available.")
  return()
endif()
```

And in fact, we need to also bail out if we *do* find Clang, but if the development package is not installed. Otherwise, the includes and `.a` files needed below won't be found.


================
Comment at: libcxx/test/tools/clang_tidy_checks/CMakeLists.txt:10-11
+if(${Clang_FOUND} AND ${LLVM_VERSION_MAJOR} GREATER_EQUAL 15)
+  message(STATUS "Found LLVM ${LLVM_PACKAGE_VERSION}")
+  message(STATUS "Include dir: ${LLVM_INCLUDE_DIRS}")
+
----------------



================
Comment at: libcxx/test/tools/clang_tidy_checks/CMakeLists.txt:11
+  message(STATUS "Found LLVM ${LLVM_PACKAGE_VERSION}")
+  message(STATUS "Include dir: ${LLVM_INCLUDE_DIRS}")
+
----------------
I would like to make it really clear that we're not using the in-tree version of headers/libraries. I suggest we add a target like this (since `find_package(Clang)` does not seem to do that):

```
add_library(clang-tidy-devel IMPORTED)
set_target_properties(clang-tidy-devel PROPERTIES IMPORTED_LOCATION "${?????}/libclangTidy.a")
target_include_directories(clang-tidy-devel INTERFACE "${LLVM_INCLUDE_DIRS}" 
                                                 "${CLANG_INCLUDE_DIRS}"
                                                 "${LLVM_MAIN_INCLUDE_DIR}/../../clang-tools-extra")
target_compile_options(clang-tidy-devel INTERFACE -fno-rtti)
```

Then, we can link `cxx-tidy` against `clang-tidy-devel`.


================
Comment at: libcxx/test/tools/clang_tidy_checks/CMakeLists.txt:13-15
+  set(CMAKE_CXX_STANDARD 20)
+
+  project(cxx-tidy)
----------------
I'm not sure why we'd need those. We already set that at the top-level.


================
Comment at: libcxx/test/tools/clang_tidy_checks/CMakeLists.txt:18
+
+  target_compile_options(cxx-tidy PRIVATE -fno-rtti)
+  target_include_directories(cxx-tidy PRIVATE
----------------
This can be removed if we have the `INTERFACE` flag suggested above.


================
Comment at: libcxx/test/tools/clang_tidy_checks/CMakeLists.txt:24
+                            )
+  set_target_properties(cxx-tidy PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
+  set(CMAKE_SHARED_MODULE_SUFFIX_CXX .dylib)
----------------
Based on the CMake documentation, this should already be the default. I don't understand why this is needed -- can we try removing it and see how it fails?


================
Comment at: libcxx/test/tools/clang_tidy_checks/CMakeLists.txt:25
+  set_target_properties(cxx-tidy PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
+  set(CMAKE_SHARED_MODULE_SUFFIX_CXX .dylib)
+endif()
----------------
Also, how about using `.plugin` instead? It's more platform agnostic.


================
Comment at: libcxx/utils/libcxx/test/features.py:24
+    return int(re.search('[0-9]+', commandOutput(cfg, ['clang-tidy --version'])).group()) >= 13 and os.path.exists(
+      next(x for x in cfg.substitutions if x[0] == '%{test-tools}')[1] + '/clang_tidy_checks/libcxx-tidy.dylib')
   except ConfigurationRuntimeError:
----------------
That doesn't work. You're manually expanding the substitution, but substitutions can contains substitutions themselves and so on. We should instead do:

```
int(re.search('[0-9]+', commandOutput(cfg, ['clang-tidy --version'])).group()) >= 13 and
runScriptExitCode(['stat %{test-tools}/clang_tidy_checks/libcxx-tidy.plugin']) == 0
```



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131963/new/

https://reviews.llvm.org/D131963