We already have a clang-tidy check for making sure that _LIBCPP_HIDE_FROM_ABI is on free functions. This patch extends this to class members. The places where we don't check for _LIBCPP_HIDE_FROM_ABI are classes for which we have an instantiation in the library.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Time | Test | |
---|---|---|
3,090 ms | libcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx::clang_tidy.sh.cpp Script:
--
: 'RUN: at line 15'; clang-tidy-16 /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/libcxx/test/libcxx/clang_tidy.sh.cpp --warnings-as-errors=* -header-filter=.* --checks='-*,libcpp-*' --load=/home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-cxx11/libcxx/test/tools/clang_tidy_checks/libcxx-tidy.plugin -- -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wunused-template -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -fno-modules
| |
4,830 ms | libcxx CI GCC 12 / C++latest > llvm-libc++-shared-gcc-cfg-in.libcxx::double_include.sh.cpp Script:
--
: 'RUN: at line 11'; /usr/bin/g++-12 -c /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/libcxx/double_include.sh.cpp -o /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/test/libcxx/Output/double_include.sh.cpp.dir/t.tmp.first.o -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wno-literal-suffix -Wno-user-defined-literals -Wno-tautological-compare -Wno-stringop-overread -Wno-stringop-overflow -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess
| |
4,800 ms | libcxx CI GCC 12 / C++latest > llvm-libc++-shared-gcc-cfg-in.libcxx::min_max_macros.compile.pass.cpp Script:
--
: 'COMPILED WITH'; /usr/bin/g++-12 /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/libcxx/min_max_macros.compile.pass.cpp -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wno-literal-suffix -Wno-user-defined-literals -Wno-tautological-compare -Wno-stringop-overread -Wno-stringop-overflow -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess -fsyntax-only
| |
4,780 ms | libcxx CI GCC 12 / C++latest > llvm-libc++-shared-gcc-cfg-in.libcxx::nasty_macros.compile.pass.cpp Script:
--
: 'COMPILED WITH'; /usr/bin/g++-12 /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/libcxx/nasty_macros.compile.pass.cpp -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wno-literal-suffix -Wno-user-defined-literals -Wno-tautological-compare -Wno-stringop-overread -Wno-stringop-overflow -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess -fsyntax-only
| |
4,810 ms | libcxx CI GCC 12 / C++latest > llvm-libc++-shared-gcc-cfg-in.libcxx::no_assert_include.compile.pass.cpp Script:
--
: 'COMPILED WITH'; /usr/bin/g++-12 /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/libcxx/no_assert_include.compile.pass.cpp -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wno-literal-suffix -Wno-user-defined-literals -Wno-tautological-compare -Wno-stringop-overread -Wno-stringop-overflow -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess -fsyntax-only
| |
View Full Test Results (410 Failed) |
Event Timeline
In general I like it, for reviewing it would have been nicer to split the patch in multiple parts:
- removal from trivial functions
- adding on non-trivial functions
That would also make it easier to see which part of the clang-tidy enables which test.
Also please don't make unrelated changes in the library code.
libcxx/include/__chrono/day.h | ||
---|---|---|
32 | Is it an issue to have this unneeded _LIBCPP_HIDE_FROM_ABI? I ask since when the class becomes non-trivial we need to add it again. | |
libcxx/include/__ranges/iota_view.h | ||
51 | Please don't hide these kind of changes in this patch. | |
libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp | ||
108 | why only testing for trivially copyable? |
libcxx/include/__chrono/day.h | ||
---|---|---|
32 | Not really. I wouldn't expect internal classes to be a problem either. It's not like we regularly change a class to be trivial which wasn't trivial before. The main reason I added this check is for tag classes and the like where it's just noise, e.g. struct some_tag { explicit some_tag() = default; } shouldn't have a _LIBCPP_HIDE_FROM_ABI IMO. | |
libcxx/include/__ranges/iota_view.h | ||
51 | What do you mean with "these kinds of changes"? I didn't think this would be a problem, given that it will never be executed anyways. | |
libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp | ||
108 | I'm not sure what you mean. The name is wrong, but I check for isTrivial(), not isTriviallyCopyable(). Was it just about the name being wrong? |
I am personally find with this patch after a few minor changes outlined in my comments. I think this is a large mechanical patch and splitting it might or might not help reviewing, I am not sure. I went through 90% of the mechanical changes to validate them and everything looked fine to me. Will let Mark LGTM the patch since he had comments.
libcxx/include/__algorithm/comp_ref_type.h | ||
---|---|---|
18 | Can your commit message please explain a bit more what the patch is about? | |
libcxx/include/__ranges/iota_view.h | ||
51 | So there were two choices here:
I think this patch should add _LIBCPP_HIDE_FROM_ABI because the reviewer's assumption is that all these changes are 100% mechanical, and this violates that assumption. We should then (trivially) make it consteval in a separate one-liner patch, cause I agree that's probably what it should have been in the first place. Or, alternatively, add consteval as a NFC prior to this patch. | |
libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp | ||
85 | ||
94 | ||
108 |