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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Could this be done with clang-query instead of a clang-tidy plugin?



================
Comment at: libcxx/include/charconv:306
 
+template <typename _Tp>
+inline _LIBCPP_HIDE_FROM_ABI to_chars_result
----------------
Why is this code moved? I am working on this header so the changes probably cause merge conflicts.

In general I dislike moving code in these kind of patches since they may cause unneeded merge conflicts.

Would it be possible to exclude this file and add a TODO so we can fix this after the consexpr version of `charconv` is done?


================
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:
----------------
ldionne wrote:
> 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
> ```
> 
I think it would be better when the user specifies the path to `clang-tidy` in CMake. Our Docker image contains multiple versions of clang-tidy; one for main and one for the release branch. To be able to use both branches the build bot configuration should be able to select the proper version.

It is already intended to remove the symlinks from the Docker file for that reason.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131963