[libcxx-commits] [PATCH] D141805: [libc++] Refactor clang-query checks to clang-tidy checks to get less obsucre error messages

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/docs/TestingLibcxx.rst:101
 These tools are:
-- clang-query
 - clang-tidy
----------------
philnik wrote:
> philnik wrote:
> > Mordante wrote:
> > > Mordante wrote:
> > > > Can we remove `clang-query` from the Dockerfile too or is it still needed there?
> > > I think there are more dependencies for `clang-tidy` with the plugins, is that correct?
> > IIRC `clang-query` part of the `clang-tools` package, which I //think// we still depend on. I might be wrong about that though. 
> Kind-of, depends on how your OS packages things. If the headers and object files are included within the same package, there are no additional dependencies. Added a comment.
It's indeed part of `clang-tools` and I think there's nothing there we use:
https://packages.debian.org/bullseye/amd64/clang-tools-11/filelist


================
Comment at: libcxx/test/tools/clang_tidy_checks/CMakeLists.txt:8
 
 set(SOURCES
+    abi_tag_on_virtual.cpp
----------------
philnik wrote:
> Mordante wrote:
> > Not entirely related to this change, but is there any documentation describing how to use this locally.
> > (I tried in the past and failed, I expect I did something wrong, but couldn't figure out what quickly.)
> No. I can write some short documentation, but I don't know how much help this will actually be, since this can be very environment-specific. I guess we could expand this with problems people had when setting it up, so others have an easier time.
If you write an initial version I'll test it on Debian and update if needed. I want to use this myself to add some additional checks; also to get a bit of experience with writing my own checks.


================
Comment at: libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp:36
+              hasAttr(clang::attr::AbiTag),
+              cxxMethodDecl(), // We have explicitly instantiated classes and some of their methods don't have these attributes
+              isDeleted(),
----------------
philnik wrote:
> Mordante wrote:
> > I assume this has to to with the list of `hasName` above. If so let's move the comment there and please elaborate a bit more why these functions are excluded. Just to make sure that it's clear why this is needed.
> > 
> > A question, is `std::ranges::__stable_sort` also added to this list? Or only names in `std::` ?
> I'm, not sure what you are referring to with `I assume this has to do with the list of hasName above`.  The functions are excluded because they can't be marked `[[gnu::always_inline]]` for various reasons.
> 
> This check only looks through functions, so `ranges::__stable_sort` isn't related.
> 
Can you then add a comment like `These functions are excluded because they can't be marked [[gnu::always_inline]].`
What happens when `std::ranges::__stable_sort` is a function? I forgot it was a cpo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141805