This allows us to reuse workarounds for compilers that don't provide the builtins or constexpr support.
Details
- Reviewers
ldionne Mordante var-const - Group Reviewers
Restricted Project - Commits
- rG72173469dd02: [libc++] Refactor char_traits
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
In general I like the approach, some remarks. I really like to see the CI once it's running again.
| libcxx/include/__string/char_traits.h | ||
|---|---|---|
| 362 | Why is this function and length not changed? Do the builtins not work with them? | |
| libcxx/include/cstring | ||
| 103 | Are these used except in the traits? | |
| libcxx/include/__string/char_traits.h | ||
|---|---|---|
| 362 | Yes, __builtin_char_memchr only works with char: https://godbolt.org/z/YvzMdzc1x | |
| libcxx/include/cstring | ||
| 103 | Not currently, but I'm working on a patch that uses __constexpr_memcmp to optimize std{,::ranges}::equal(). | |
| libcxx/include/__string/char_traits.h | ||
|---|---|---|
| 227 | I would make this explicit, and I'd use int instead of int_type since int is what memchr takes. | |
LGTM modulo some nits.
| libcxx/include/__string/char_traits.h | ||
|---|---|---|
| 215 | This is odd, I did leave a message here, but it's gone :-/ How about removing this test? I tested in godbolt and Clang and GCC for -O0 and -O2 this generated no code. | |
| libcxx/test/libcxx/strings/c.strings/constexpr.cwchar.compile.pass.cpp | ||
| 10 | ||
- Address comments
- Try to fix CI
| libcxx/include/__string/char_traits.h | ||
|---|---|---|
| 215 | Maybe? I've tried and for me it generates code: https://godbolt.org/z/54v99TMo9. Though it's arguably not worth the extra four instructions and branch prediction slot to optimize for comparing nothing. I mainly kept it because I thought it was UB to call memcmp with null pointers, but I can't find any evidence of that, so removing I'm it. | |
Remove static_assert again, since it also fires when not actually calling __constexpr_memcmp during constant evaluation
We are seeing build breakages in Fuchsia's builders after this change was landed and this change is in the blamelist and looks directly related:
[37031/289776](267) CXX kernel.efi_x64/obj/src/lib/llvm-profdata/libllvm-profdata.llvm-profdata.cc.o
FAILED: kernel.efi_x64/obj/src/lib/llvm-profdata/libllvm-profdata.llvm-profdata.cc.o
../../../recipe_cleanup/clangehe1kz7e/bin/clang++ -MD -MF kernel.efi_x64/obj/src/lib/llvm-profdata/libllvm-profdata.llvm-profdata.cc.o.d -o kernel.efi_x64/obj/src/lib/llvm-profdata/libllvm-profdata...
In file included from ../../src/lib/llvm-profdata/llvm-profdata.cc:5:
In file included from ../../src/lib/llvm-profdata/include/lib/llvm-profdata/llvm-profdata.h:11:
In file included from ../../../recipe_cleanup/clangehe1kz7e/include/c++/v1/string_view:220:
../../../recipe_cleanup/clangehe1kz7e/include/c++/v1/__string/char_traits.h:286:17: error: no member named '__constexpr_wmemcmp' in namespace 'std'; did you mean '__constexpr_memcmp'?
return std::__constexpr_wmemcmp(__s1, __s2, __n);
~~~~~^
../../../recipe_cleanup/clangehe1kz7e/include/c++/v1/cstring:119:1: note: '__constexpr_memcmp' declared here
__constexpr_memcmp(const _Tp* __lhs, const _Tp* __rhs, size_t __count) {
^
In file included from ../../src/lib/llvm-profdata/llvm-profdata.cc:5:
In file included from ../../src/lib/llvm-profdata/include/lib/llvm-profdata/llvm-profdata.h:11:
In file included from ../../../recipe_cleanup/clangehe1kz7e/include/c++/v1/string_view:220:
../../../recipe_cleanup/clangehe1kz7e/include/c++/v1/__string/char_traits.h:290:17: error: no member named '__constexpr_wcslen' in namespace 'std'
return std::__constexpr_wcslen(__s);
~~~~~^
../../../recipe_cleanup/clangehe1kz7e/include/c++/v1/__string/char_traits.h:297:17: error: no member named '__constexpr_wmemchr' in namespace 'std'
return std::__constexpr_wmemchr(__s, __a, __n);
~~~~~^
3 errors generated.
clang++: error: failing because '-gen-reproducer' is used
Fuchsia clang version 16.0.0 (https://llvm.googlesource.com/llvm-project fe9e442c57f3b50ec7ad5f01e191745aa3156b4e)
Target: x86_64-unknown-windows-msvc
Thread model: posix
InstalledDir: ../../../recipe_cleanup/clangehe1kz7e/bin
clang++: note: diagnostic msg:
********************
PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang++: note: diagnostic msg: clang-crashreports/llvm-profdata-960ad3.cpp
clang++: note: diagnostic msg: clang-crashreports/llvm-profdata-960ad3.sh
clang++: note: diagnostic msg:
********************Could you take a look?
If it takes a while to fix, could you revert the change first?
Looks like it is Fuchsia's own issue as it overrides libcxx headers in kernel (https://fuchsia.googlesource.com/fuchsia/+/658bbc3cef7f340c058dde67c3d5dd2be4084bdd/zircon/kernel/lib/ktl/include/cwchar). You don't need to take any action. Sorry for the trouble.
I would check if that workaround is still needed. You should be able to configure the library with LIBCXX_ENABLE_WIDE_CHARACTERS=OFF at CMake time to make sure that the library doesn't rely on this header.
This is odd, I did leave a message here, but it's gone :-/
How about removing this test? I tested in godbolt and Clang and GCC for -O0 and -O2 this generated no code.
(If wanted I can recreate the example in godbolt.)