Details
- Reviewers
mclow.lists EricWF rsmith ldionne - Group Reviewers
Restricted Project
Diff Detail
- Repository
- rG LLVM Github Monorepo
Time | Test | |
---|---|---|
4,040 ms | libcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx::clang_tidy.sh.cpp Script:
--
: 'RUN: at line 12'; clang-tidy /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/libcxx/clang_tidy.sh.cpp --warnings-as-errors=* -header-filter=.* -- -Wweak-vtables -Wno-unknown-warning-option -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -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_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fno-modules
| |
4,040 ms | libcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx::clang_tidy.sh.cpp Script:
--
: 'RUN: at line 12'; clang-tidy /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/libcxx/clang_tidy.sh.cpp --warnings-as-errors=* -header-filter=.* -- -Wweak-vtables -Wno-unknown-warning-option -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -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_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fno-modules
| |
4,040 ms | libcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx::clang_tidy.sh.cpp Script:
--
: 'RUN: at line 12'; clang-tidy /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/libcxx/clang_tidy.sh.cpp --warnings-as-errors=* -header-filter=.* -- -Wweak-vtables -Wno-unknown-warning-option -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -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_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fno-modules
| |
4,040 ms | libcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx::clang_tidy.sh.cpp Script:
--
: 'RUN: at line 12'; clang-tidy /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/libcxx/clang_tidy.sh.cpp --warnings-as-errors=* -header-filter=.* -- -Wweak-vtables -Wno-unknown-warning-option -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -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_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fno-modules
| |
4,040 ms | libcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx::clang_tidy.sh.cpp Script:
--
: 'RUN: at line 12'; clang-tidy /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/libcxx/clang_tidy.sh.cpp --warnings-as-errors=* -header-filter=.* -- -Wweak-vtables -Wno-unknown-warning-option -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -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_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fno-modules
| |
View Full Test Results (21,280 Failed) |
Event Timeline
I've had a revision locally for a long long time. It doesn't pass everything
yet, but I've managed to get it down to ~10 failing tests with a few more hacks
locally.
Some quick review of the the acutal changes to the headers
libcxx/include/memory | ||
---|---|---|
884–885 | I am wondering whether we should reuse the simplementation of std::copy_n here (or at the call site for what its worth) | |
884–885 | As said above, we should be able to reuse the internals of std::copy_n here | |
884–885 | I believe the inline is superfluous with the _LIBCPP_CONSTEXPR_AFTER_CXX17 Appears throughout | |
884–885 | From here and below there is _LIBCPP_CONSTEXPR_AFTER_CXX11 rather than _LIBCPP_CONSTEXPR_AFTER_CXX17 Also the ordering of _LIBCPP_INLINE_VISIBILITY and _LIBCPP_CONSTEXPR_AFTER_CXX11 is swapped. | |
libcxx/include/vector | ||
332–333 | Many inline below here |
Rebase onto main.
@miscco's comments have not been addressed yet -- I only did the minimal amount of work to fix merge conflicts with main.
Note that not all tests have been constexpr-ified yet, and the ones that have been constexpr-ified are not all passing. Transferring to @philnik as discussed offline.
- Fix tests
libcxx/include/memory | ||
---|---|---|
884–885 | I think this and the comment below have been addressed. But I'm not 100% sure what you were referring to exactly. Could you comment again at the correct line if the comments haven't been addressed? | |
884–885 | Sorry, I don't know what you are referring to again. I guess they have been addressed since I can't see any _LIBCPP_CONSTEXPR_AFTER_CXX11 of inline in the diff. | |
libcxx/include/vector | ||
332–333 | Again, I guess it has been addressed? |
libcxx/include/__bit_reference | ||
---|---|---|
358–359 | I think we should always call std::fill_n here instead. Indeed, both are equivalent, but std::fill_n is nicer for various reasons. I know fill_n doesn't end up calling memset directly, however the compiler will end up calling memset (when optimized). IIRC, @var-const was actually about to add this optimization to fill_n explicitly, when we realized it didn't have an impact on the code generation. @var-const can you confirm? Also, I think we need to add a comment in std::fill_n that explains this. I think it can be done in this patch since we're switching from memset to fill_n. This comment applies elsewhere in this patch too. | |
497 | Why is this required? | |
957–960 | Instead, I would begin the lifetime of the elements inside __bit_array's constructor if we are during constant evaluation. That way you shouldn't need to change this code at all. | |
libcxx/include/__memory/construct_at.h | ||
40–41 | Please match the style of the class = decltype above, just for consistency. | |
46 | Let's add the _LIBCPP_ASSERT here too. | |
libcxx/include/__utility/move.h | ||
40–49 ↗ | (On Diff #436255) | I don't think this diff is needed in this patch, see my comment below. |
libcxx/include/memory | ||
884–885 | Why are we using std::__copy here? Let's use std::copy instead. | |
884–885 | Given that __begin2 = std::__copy(__begin1, __end1, __begin2); does not compile, I think this overload is never used. Can you please confirm this by adding static_assert(sizeof(_Alloc) == 0, "") and running the tests? If the tests don't fail, then this is a dead code path and we'll need to investigate it. Edit Well, it looks like we've got enable_if<cond>::type above as a template parameter. IOW, we're declaring a NTTP of type void, and that's never valid. 🤦🏼♂️ Clang should really tell us about that. Let's fix it in a separate patch prior to this one. | |
884–885 | I think this should all just be uninitialized_copy (or a __ version of it). | |
885 | I don't think we *need* to make this change to enable constexpr vector, right? If not, let's pull it into a separate patch, cause I'd like to have a discussion about it. Otherwise, can you please explain why it's needed? |
libcxx/include/__bit_reference | ||
---|---|---|
358–359 | I can confirm that working on https://reviews.llvm.org/D118329, I found that the compiler would replace uninitialized_fill{,_n} with calls to memset in an optimized build, which made it unnecessary to write that optimization explicitly. I would expect fill_n to work similarly, but I have only tried with the uninitialized algorithms. FWIW, I have a lingering suspicion that there might be some corner cases (related to aliasing, perhaps) where the compiler would fail to do the optimization, but it was absolutely reliable in all the (simple) cases I checked. |