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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 2 inline comments as done.
philnik added a comment.

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

> 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.

I didn't mean that we should never upstream it. I meant that if we didn't add a clang-tidy plugin that we'd have to wait up to six months before enabling a check. I'd also rather wait to upstream this check, since it's not very well tested yet. I'd like to check if it catches all ADL that the current ADL tests catch and if not improve the check in a follow-up.



================
Comment at: libcxx/include/charconv:306
 
+template <typename _Tp>
+inline _LIBCPP_HIDE_FROM_ABI to_chars_result
----------------
Mordante wrote:
> 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?
I'll do that if this lands before your patches, which I don't really expect. There is a reason I didn't consider the comment done yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131963