[libcxx-commits] [PATCH] D129968: [libc++] Add a bunch of missing _LIBCPP_HIDE_FROM_ABI

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D129968#3693265 <https://reviews.llvm.org/D129968#3693265>, @philnik wrote:

>> I think we can find more use-cases for AST-matchers! Actually I have some use-cases on my todo list ;-)
>
> I'm sure there are. I've got a few thoughts there too. What exactly would you like to have matchers for? I'm happy to implement a few more.

Validate all `main` functions in the tests have the proper signature and have a `return 0;`.



================
Comment at: libcxx/test/libcxx/clang_query.sh.cpp:9
+
+// REQUIRES: has-clang-query
+
----------------
philnik wrote:
> var-const wrote:
> > Mordante wrote:
> > > https://buildkite.com/llvm-project/libcxx-ci/builds/12164#01820d36-8444-4b5e-b2f5-a3a866703ed6
> > > lists the test as unsupported.
> > > 
> > > Our Docker doesn't install `clang-tools-$LLVM_LATEST_VERSION`.
> > > 
> > > Can you update our Docker file in a separate review and rebase this one after it passes.
> > > Maybe it's even better to split this patch in the missing `_LIBCPP_HIDE_FROM_ABI` and the test improvements. Then you can land the first part quickly without having the risk of merge conflicts in that part.
> > > 
> > Can you add a comment to explain the purpose of this file?
> What exactly are you asking for? This file is just supposed to run `clang-query` on the headers.
Can you add that as comment and add a link to the README.md you added?


================
Comment at: libcxx/utils/libcxx/test/features.py:29
+  try:
+    return int(re.search('[0-9]+', commandOutput(cfg, ['clang-query --version'])).group()) >= 13
+  except ConfigurationRuntimeError:
----------------
philnik wrote:
> philnik wrote:
> > Mordante wrote:
> > > Mordante wrote:
> > > > Does this also find `clang-query-15`? This is the default name used in the `apt.llvm.org` so I would like that to work out of the box.
> > > Can you update the developer documentation with this tool (I think clang-tidy) also isn't listed.
> > No, it doesn't find `clang-query-15`. I also don't know how this could be implemented in a way that finds different versions. We don't know which version is the correct one if we find multiple. IMO we shouldn't try to find some executable but rather ask the user to put a `clang-query` into the path. That's a lot safer. With the compiler it's a different story, since we get that through CMake.
> What exactly do you mean? Where do we have any developer documentation w.r.t. tooling?
https://libcxx.llvm.org/TestingLibcxx.html should list the dependencies to be able to execute the tests.


================
Comment at: libcxx/utils/libcxx/test/features.py:29
+  try:
+    return int(re.search('[0-9]+', commandOutput(cfg, ['clang-query --version'])).group()) >= 13
+  except ConfigurationRuntimeError:
----------------
Mordante wrote:
> philnik wrote:
> > philnik wrote:
> > > Mordante wrote:
> > > > Mordante wrote:
> > > > > Does this also find `clang-query-15`? This is the default name used in the `apt.llvm.org` so I would like that to work out of the box.
> > > > Can you update the developer documentation with this tool (I think clang-tidy) also isn't listed.
> > > No, it doesn't find `clang-query-15`. I also don't know how this could be implemented in a way that finds different versions. We don't know which version is the correct one if we find multiple. IMO we shouldn't try to find some executable but rather ask the user to put a `clang-query` into the path. That's a lot safer. With the compiler it's a different story, since we get that through CMake.
> > What exactly do you mean? Where do we have any developer documentation w.r.t. tooling?
> https://libcxx.llvm.org/TestingLibcxx.html should list the dependencies to be able to execute the tests.
> No, it doesn't find `clang-query-15`. I also don't know how this could be implemented in a way that finds different versions. We don't know which version is the correct one if we find multiple. IMO we shouldn't try to find some executable but rather ask the user to put a `clang-query` into the path. That's a lot safer. With the compiler it's a different story, since we get that through CMake.

It's already possible to select the wanted compiler and several other features using CMake options. I really would like to make this a configuration option too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129968