Details
- Reviewers
ldionne philnik - Group Reviewers
Restricted Project Restricted Project - Commits
- rGeb12ad9d7ff6: [libc++][CI] Updates GCC to version 12.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/utils/libcxx/test/params.py | ||
---|---|---|
33 | This disabled leads to a lot of errors in C++11 with GCC especially in the string tests. The issue is the code is tested constexpr starting with C++20. Therefore the compiler can deduce that in C++11 mode the function never is constexpr and thus the test always false. In file included from /home/libcxx-builder/.buildkite-agent/builds/28dd04a444a9-1/llvm-project/libcxx-ci/libcxx/test/std/strings/basic.string/string.modifiers/string_copy/copy.pass.cpp:20: /home/libcxx-builder/.buildkite-agent/builds/28dd04a444a9-1/llvm-project/libcxx-ci/libcxx/test/std/strings/basic.string/string.modifiers/string_copy/copy.pass.cpp: In function 'void test(S, typename S::value_type*, typename S::size_type, typename S::size_type)': /home/libcxx-builder/.buildkite-agent/builds/28dd04a444a9-1/llvm-project/libcxx-ci/libcxx/test/support/test_macros.h:146:68: error: 'std::is_constant_evaluated' always evaluates to false in a non-'constexpr' function [-Werror=tautological-compare] 146 | # define TEST_IS_CONSTANT_EVALUATED __builtin_is_constant_evaluated() | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~ /home/libcxx-builder/.buildkite-agent/builds/28dd04a444a9-1/llvm-project/libcxx-ci/libcxx/test/std/strings/basic.string/string.modifiers/string_copy/copy.pass.cpp:38:15: note: in expansion of macro 'TEST_IS_CONSTANT_EVALUATED' 38 | else if (!TEST_IS_CONSTANT_EVALUATED) | ^~~~~~~~~~~~~~~~~~~~~~~~~~ I see several possible solutions
I think solution 1 would be the best, @ldionne, @philnik WDYT? |
libcxx/utils/libcxx/test/params.py | ||
---|---|---|
33 | Do you know if the TEST_IS_CONSTANT_EVALUATED cases are the only ones? I think we should refactor them anyways. In hindsight this should have never been a macro. I would change it to something like namespace test { enum Version { Cpp20, Cpp23, }; constexpr bool is_constant_evaluated_impl() { #if TEST_STD_VER >= 20 && defined(__cpp_lib_is_constant_evaluated) && __cpp_lib_is_constant_evaluated >= 201811L return std::is_constant_evaluated(); #else return false; #endif } constexpr bool is_constant_evaluated(Version ver) { switch (ver) { case Cpp20: return is_constant_evaluated_impl() && TEST_STD_VER >= 20; case Cpp23: return is_constant_evaluated_impl() && TEST_STD_VER >= 23; } assert(false); } } This allows us to replace the current uses of TEST_IS_CONSTANT_EVALUATED with test::is_constant_evaluated(test::Cpp20) and for constexpr std::unique_ptr use test::is_constant_evaluated(test::Cpp23). I also think that we should just remove the __builtin_is_constant_evaluated() part. It's unused anyways currently. If we need it at any point we can still add it later. According to https://godbolt.org/z/cqdv88xez GCC is also happy with this reworked version. |
libcxx/utils/libcxx/test/params.py | ||
---|---|---|
33 | I'm not sure whether these are the only ones. If we go with your approach or TEST_IS_CONSTANT_EVALUATED_AFTER_11 it should be a separate patch. Then this patch needs to be on top of that one. Then it's easier to investigate whether more work is needed. Interesting approach. I'll give this one some more thought. |
libcxx/utils/libcxx/test/params.py | ||
---|---|---|
33 | I like @philnik 's approach, but I am not sure it will resolve the issues. Technically, GCC could still figure out that we're returning false if it evaluates the is_constant_evaluated(Cpp20) expression deep enough. But yeah, I'd definitely welcome a patch that tries to do this -- personally I've never been a huge fan of the TEST_IS_CONSTANT_EVALUATED macro anyways. |
libcxxabi/test/catch_member_function_pointer_02.pass.cpp | ||
---|---|---|
18 | I'll remove gcc-11 before landing or with the next iteration of this patch. |
LGTM.
libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.char_array.pass.cpp | ||
---|---|---|
10–11 | Couldn't you just use std::copy_n for this? |
Thanks for the review!
libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.char_array.pass.cpp | ||
---|---|---|
10–11 | I probably can; however when I wrote the test (in the GCC-10 era) there were a lot more issues. But I want to look whether I can refactor this test, I learned a bit more about "string literals" as template argument. |
Thanks for keeping the CI up-to-date! This LGTM, however I would support landing this with -Wno-tautological-compare in the test suite to get this unblocked until (or if) we find a definitive solution for the issue (since I'm not sure that D127978 is what we want).
Couldn't you just use std::copy_n for this?