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

I love this patch! 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. I think we can find more use-cases for AST-matchers! Actually I have some use-cases on my todo list ;-)

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

> @Mordante @var-const @huixie90 @ldionne Please tell me which parts of this PR conflict with any of your PRs. I think it's mostly be the algorithms, especially `sort`.

Thanks, I'm working on regex too, but I see no big conflicts. Nor in other code I'm working on.

> Notable exceptions to adding `_LIBCPP_HIDE_FROM_ABI` are `__sort`, `__insertion_sort_incomplete` and `operator+(const _CharT*, const basic_string&)`. These have specializations in the dylib and I have them marked `_LIBCPP_HIDDEN` instead because of that.

Thanks for pointing that out.

In general I'm happy with the changes, but several comments. Mainly regarding the infrastructure. As mentioned in the comments I would encourage you to split the fixes from the infrastructure.



================
Comment at: libcxx/include/__format/formatter_floating_point.h:106
 template <floating_point _Fp>
-static constexpr size_t __float_buffer_size(int __precision) {
   using _Traits = __traits<_Fp>;
----------------
philnik wrote:
> @Mordante I'm guessing this was accidentally marked `static`?
Yes, nice catch!


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



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


================
Comment at: libcxx/test/libcxx/clang_query/hide_from_abi_or_visible.query:1
+match functionDecl(
+        allOf(
----------------
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


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129968