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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

In D131963#3754059 <https://reviews.llvm.org/D131963#3754059>, @Mordante wrote:

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

No. With a clang-tidy plugin we can get all the information in the AST, which we can't through clang-query. See `AST_MATCHER(...)` in `robust_against_adl.cpp` for examples. Using a clang-tidy plugin also has the benefit of getting proper error messages and (potentially) IDE integration. If we're ambitious maybe even fixit hints.



================
Comment at: libcxx/include/charconv:306
 
+template <typename _Tp>
+inline _LIBCPP_HIDE_FROM_ABI to_chars_result
----------------
Mordante wrote:
> 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?
It's moved because it calls the `__to_chars_itoa` in line 295. This wasn't checked before until the functions is called I think.


================
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:
----------------
Mordante wrote:
> 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.
Ok, but what has that to do with this change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131963