[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 a comment.

I like the removal of a dependency, especially since clang-tidy shows better diagnostics.
Some questions and remarks.



================
Comment at: libcxx/docs/TestingLibcxx.rst:101
 These tools are:
-- clang-query
 - clang-tidy
----------------
Can we remove `clang-query` from the Dockerfile too or is it still needed there?


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


================
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(); }
 
----------------
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?


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


================
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(),
----------------
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::` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141805