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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D131963#3754322 <https://reviews.llvm.org/D131963#3754322>, @philnik wrote:

> In D131963#3754318 <https://reviews.llvm.org/D131963#3754318>, @avogelsgesang wrote:
>
>> is there a reason why we add our own clang-tidy plugin instead of upstreaming this to `clang-tidy` itself? I could imagine that other library developers could also benefit from this clang-tidy check, if we make the list of allowed customization points a configuration option
>
> We can definitely do this for some checks, but I don't think it makes sense for all checks. I'd like to extend our `_LIBCPP_HIDE_FROM_ABI` check for example, but that check probably doesn't make sense for any other project. Upstreaming a check would also mean that we'd have to wait up to six months before being able to enforce it.

I too prefer to upstream useful changes; `_LIBCPP_HIDE_FROM_ABI` is indeed not one of them. I disagree with your assessment. We can do both, implement it locally and upstream it at the same time. That way we can use it directly and remove the local version half a year later.



================
Comment at: libcxx/include/charconv:306
 
+template <typename _Tp>
+inline _LIBCPP_HIDE_FROM_ABI to_chars_result
----------------
philnik wrote:
> 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.
Can we exclude this file as asked above?


================
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:
----------------
philnik wrote:
> 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?
I thought it changed the clang-tidy version use.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131963