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

[Louis Dionne via Phabricator via libcxx-commits]

philnik created this revision.
Herald added subscribers: miyuki, arichardson, mgorny.
Herald added a project: All.
ldionne added a comment.
philnik updated this revision to Diff 455594.
philnik marked 2 inline comments as done.
ldionne published this revision for review.
Herald added a reviewer: jdoerfert.
Herald added subscribers: libcxx-commits, sstefan1.
Herald added a project: libc++.
Herald added a reviewer: libc++.

This is really interesting! If you can get me more details about what `FindClang.cmake` looks like (I wasn't able to find after searching quickly), I might be able to provide more guidance on how to integrate it.


philnik added a comment.

- Address some comments



================
Comment at: libcxx/test/configs/cmake-bridge.cfg.in:33
 config.substitutions.append(('%{executor}', '@LIBCXX_EXECUTOR@'))
+config.substitutions.append(('%{libcpp-clang-tidy-path}', '@LIBCXX_CLANG_TIDY_PATH@'))
----------------
Instead, maybe we could introduce a directory where we install build tools like this and use `--load=%{build-tools}/libcxx-tidy.*`?


================
Comment at: libcxx/test/libcxx/clang_tidy_checks/CMakeLists.txt:16
+
+  llvm_add_library(libcxx_tidy MODULE ${SOURCES}
+                   PLUGIN_TOOL clang-tidy)
----------------
This relies on the fact that we used `AddLLVM` in the top-level CMake. This is like a weird hybrid between using the system-installed `clang-tidy` and the one we have in the monorepo. I think we should do the former. I would expect something like this (pseudo code):

```
// In the Dockerfile
sudo apt-get install clang-devel # that installs development headers and stuff for clang/clang-tidy/etc

// In the CMake
find_package(Clang)

add_library(libcxx_tidy SHARED ${SOURCES})
target_link_libraries(libcxx_tidy PUBLIC Clang::clang-tidy-lib)
```

I actually expect that something like this should be all that's needed if Clang does provide a working `FindClang.cmake` (IDK whether it does, though).


================
Comment at: libcxx/utils/libcxx/test/features.py:147
           when=_hasSuitableClangQuery),
+  Feature(name='has-libcxx-tidy',
+          when=lambda cfg: os.path.isfile(next(x for x in cfg.substitutions if x[0] != '%{libcpp-clang-tidy-path}')[1])),
----------------
Instead, `has-clang-tidy` should represent whether we have clang-tidy *AND* the development headers have been installed. That way, we can check for `has-clang-tidy` only in the tests.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131963

Files:
  libcxx/CMakeLists.txt
  libcxx/include/__algorithm/ranges_search_n.h
  libcxx/include/__algorithm/sort.h
  libcxx/include/__bit_reference
  libcxx/include/__chrono/duration.h
  libcxx/include/__chrono/hh_mm_ss.h
  libcxx/include/__chrono/time_point.h
  libcxx/include/__compare/common_comparison_category.h
  libcxx/include/__compare/partial_order.h
  libcxx/include/__compare/strong_order.h
  libcxx/include/__compare/weak_order.h
  libcxx/include/__filesystem/path.h
  libcxx/include/__format/buffer.h
  libcxx/include/__format/format_arg_store.h
  libcxx/include/__format/format_string.h
  libcxx/include/__format/formatter_floating_point.h
  libcxx/include/__format/formatter_integral.h
  libcxx/include/__format/formatter_output.h
  libcxx/include/__format/parser_std_format_spec.h
  libcxx/include/__functional/hash.h
  libcxx/include/__hash_table
  libcxx/include/__iterator/common_iterator.h
  libcxx/include/__iterator/iter_move.h
  libcxx/include/__iterator/iter_swap.h
  libcxx/include/__locale
  libcxx/include/__memory/shared_ptr.h
  libcxx/include/__memory/uninitialized_algorithms.h
  libcxx/include/__numeric/midpoint.h
  libcxx/include/__random/binomial_distribution.h
  libcxx/include/__random/uniform_int_distribution.h
  libcxx/include/__split_buffer
  libcxx/include/__string/char_traits.h
  libcxx/include/__type_traits/conjunction.h
  libcxx/include/__type_traits/is_callable.h
  libcxx/include/__type_traits/is_implicitly_default_constructible.h
  libcxx/include/__type_traits/is_valid_expansion.h
  libcxx/include/__utility/declval.h
  libcxx/include/atomic
  libcxx/include/barrier
  libcxx/include/charconv
  libcxx/include/cmath
  libcxx/include/complex
  libcxx/include/deque
  libcxx/include/experimental/functional
  libcxx/include/experimental/simd
  libcxx/include/format
  libcxx/include/fstream
  libcxx/include/future
  libcxx/include/iomanip
  libcxx/include/ios
  libcxx/include/istream
  libcxx/include/locale
  libcxx/include/math.h
  libcxx/include/mutex
  libcxx/include/new
  libcxx/include/ostream
  libcxx/include/regex
  libcxx/include/scoped_allocator
  libcxx/include/semaphore
  libcxx/include/string
  libcxx/include/string_view
  libcxx/include/thread
  libcxx/include/tuple
  libcxx/include/unordered_map
  libcxx/include/unordered_set
  libcxx/include/valarray
  libcxx/include/variant
  libcxx/test/CMakeLists.txt
  libcxx/test/configs/cmake-bridge.cfg.in
  libcxx/test/libcxx/clang_tidy.sh.cpp
  libcxx/test/tools/CMakeLists.txt
  libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
  libcxx/test/tools/clang_tidy_checks/libcpp_module.cpp
  libcxx/test/tools/clang_tidy_checks/robust_against_adl.cpp
  libcxx/test/tools/clang_tidy_checks/robust_against_adl.hpp
  libcxx/utils/ci/run-buildbot
  libcxx/utils/libcxx/test/features.py

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D131963.455594.patch
Type: text/x-patch
Size: 223692 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20220825/678b0adb/attachment-0001.bin>