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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/docs/TestingLibcxx.rst:101
 These tools are:
-- clang-query
 - clang-tidy
----------------
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. 


================
Comment at: libcxx/docs/TestingLibcxx.rst:101
 These tools are:
-- clang-query
 - clang-tidy
 
----------------
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.


================
Comment at: libcxx/include/__memory_resource/monotonic_buffer_resource.h:83
 
-  _LIBCPP_HIDE_FROM_ABI ~monotonic_buffer_resource() override { release(); }
+  _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~monotonic_buffer_resource() override { release(); }
 
----------------
Mordante wrote:
> Is this intended in this patch? It feels completely unrelated to test refactoring.
> If it's not related can you move this to a new review and let this one depend on it?
D141864


================
Comment at: libcxx/test/tools/clang_tidy_checks/CMakeLists.txt:8
 
 set(SOURCES
+    abi_tag_on_virtual.cpp
----------------
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.


================
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(),
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141805