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

[Nikolas Klauser via Phabricator via libcxx-commits]

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

> I can't remember how often I forgot to add these macros, in fact I forgot it today. So when a tool can help us it's great.

Exactly my thoughts. :-) The only thing that would be even better is to have these as clang-tidy checks so people get screamed at by their IDE. But that's a lot harder to accomplish properly and way less flexible, so it may not be feasible for all matchers.

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



================
Comment at: libcxx/include/__string/char_traits.h:168
 template <class _CharT>
-static inline _LIBCPP_CONSTEXPR_AFTER_CXX17
+_LIBCPP_HIDE_FROM_ABI static inline _LIBCPP_CONSTEXPR_AFTER_CXX17
 _CharT* __char_traits_move(_CharT* __dest, const _CharT* __source, size_t __n) _NOEXCEPT
----------------
huixie90 wrote:
> is this meant to be `static`?
I don't think so. I'll make another PR to search for this pattern later.


================
Comment at: libcxx/test/libcxx/clang_query.sh.cpp:9
+
+// REQUIRES: has-clang-query
+
----------------
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.


================
Comment at: libcxx/test/libcxx/clang_query.sh.cpp:11
+
+// RUN: clang-query -f %S/clang_query/hide_from_abi_or_visible.query %s --use-color -- -Wno-unknown-warning-option %{compile_flags} -fno-modules > %t.output
+// RUN: cat %t.output
----------------
Mordante wrote:
> I really like to use variable for `clang-query` like we do with our compiler. Now the name is hard-coded and may not work on systems where the tool is named differently. (See my comment in `features.py` too.)
See my comment in `features.py`. (Marking as done to keep the discussion in one place)


================
Comment at: libcxx/test/libcxx/clang_query/hide_from_abi_or_visible.query:1
+match functionDecl(
+        allOf(
----------------
Mordante wrote:
> Please add a copyright header.
> 
> I'm familiar with clang-query and the AST, however I fear that people not familiar with this are lost at this code.
> Can you add a short description what the query does and a link to the documentation, for example https://clang.llvm.org/docs/LibASTMatchersReference.html
This is actually only the second time I'm working with matchers and I find it crazy how intuitive it is to write C++ AST matchers. Anyways, a comment explaining what the intention of this matcher is is probably a good idea. I've added a more general description of what this is in a README, since I suspect that part would be copy-pasted into every matcher otherwise.


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


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129968