Initial patch for implementing https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2520r0.html
Details
- Reviewers
philnik ldionne huixie90 EricWF fsb4000 phyBrackets - Group Reviewers
Restricted Project - Commits
- rG813e1da9749a: [libc++] implement move_iterator<T*> should be a random access iterator \n…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Time | Test | |
---|---|---|
4,240 ms | libcxx CI Clang-cl (no vcruntime exceptions) > llvm-libc++-shared-no-vcruntime-clangcl-cfg-in.std/thread/thread_condition/thread_condition_condvar::notify_all.pass.cpp Script:
--
: 'COMPILED WITH'; 'C:/Program Files/LLVM/bin/clang-cl.exe' C:\ws\w7\llvm-project\libcxx-ci\libcxx\test\std\thread\thread.condition\thread.condition.condvar\notify_all.pass.cpp --driver-mode=g++ --target=x86_64-pc-windows-msvc -nostdinc++ -I C:/ws/w7/llvm-project/libcxx-ci/build/clang-cl-no-vcruntime/include/c++/v1 -I C:/ws/w7/llvm-project/libcxx-ci/build/clang-cl-no-vcruntime/include/c++/v1 -I C:/ws/w7/llvm-project/libcxx-ci/libcxx/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_HAS_EXCEPTIONS=0 -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-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 -Werror=thread-safety -Wuser-defined-warnings -DTEST_WINDOWS_DLL -nostdlib -L C:/ws/w7/llvm-project/libcxx-ci/build/clang-cl-no-vcruntime/lib -lc++ -lmsvcrt -lmsvcprt -loldnames -o C:\ws\w7\llvm-project\libcxx-ci\build\clang-cl-no-vcruntime\test\std\thread\thread.condition\thread.condition.condvar\Output\notify_all.pass.cpp.dir\t.tmp.exe
|
Event Timeline
It still failing on the ranges.min.max.pass.cpp, we need to guard that too for passing the CI. Or Is there any other work around?
Hi.
I started https://reviews.llvm.org/D142841 because I missed your revision.
I abadonned my revision.
I think you need to do such changes that I did in:
libcxx/docs/ReleaseNotes.rst
libcxx/docs/Status/Cxx2bPapers.csv
libcxx/include/iterator
Feel free to use my revision.
libcxx/test/std/iterators/predef.iterators/move.iterators/move.iterator/types.pass.cpp | ||
---|---|---|
19 |
libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp failed because of a bug in our ranges::minmax implementation.
I will prepare a fix in few minutes.
For an update on the patch, this patch is currently for _LIBCPP_STD_VER > 20 , and tests cases fixed accordingly to pass the CI , if there is anything that needs to be change, let me know!
Please remove the format-only changes. This makes it incredibly hard to see what you actually changed.
Hey @philnik , sorry for the format changes but this is what i can do best for now , not sure about the removing all the format changes!
for ignoring the whitespace changes used git diff -b that's why it's failing for now but will change it once you review the basic structure of the patch
I pushed my commit: https://github.com/llvm/llvm-project/commit/0a0d58f5465a2524ec9ed10ed0bb277cab5a180c
Now you have to rebase your patch and then you don't need
#if TEST_STD_VER <= 20 ... #endif
in ranges.minmax.pass.cpp
Actually that code block was removed at all.
patch application failed
How did you create your diff?
I create my diffs like this:
git diff -U999999 main > 1.txt
and then I upload 1.txt
Build 327893: pre-merge checks patch application failed
Could you upload your branch to your github? (https://github.com/phyBrackets/llvm-project-1)
I will try to generate the correct diff.
Thanks, I see something really cursed happened.
https://github.com/phyBrackets/llvm-project-1/tree/newB
This branch is 2972 commits ahead, 3044 commits behind llvm:main.
You could try upload my 1.txt file as a revision, maybe we can progress this way...
libcxx/docs/FeatureTestMacroTable.rst | ||
---|---|---|
263 | CI failed because extra space there |
Microsoft STL treats this paper as a DR and I agree with Casey Carter that the code breakage will be minimal because the move_iterators are still input iterators in C++20.
libc++ already gives more guaranties for things than the standard says, for example for the 'std::filesystem::path::iterator'.
from https://github.com/microsoft/STL/pull/2958#discussion_r931710527
This paper is effectively an admission that artificially depressing the category of move_iterator was a failed experiment. There's no value in preserving the undesired behavior in C++20 mode, and we suspect the potential for breakage to be very small given that move_iterators are still input iterators even if they now sometimes satisfy stronger concepts. We therefore decided to implement this change in all pertinent language modes.
What do other reviewers think?
@jwakely Does libstdc++ implement P2520R0 in C++20 as a DR too?
My desire here is to align the various implementations.
We didn't implement this yet, but Casey's position makes sense to me.
I'm happy to do this for C++20 if you are doing it.
Ok, let's all do it then.
@phyBrackets Can you please add the note in another PR? This was committed sooner than I expected.
libcxx/docs/Status/Cxx2bPapers.csv | ||
---|---|---|
87 | Please add a note that we implement P2520R0 as a DR in C++20 as well. |
FYI it seems that commit message does have a slightly different
Differntial Revision- https://reviews.llvm.org/D135248
part in the message. Please use the proper form next time, then Phabricator will automatically close the revision.
Can you close it manually?
Hey @Mordante , Sorry for it! I'm not sure how to close it manually, I tried Add Action here but I cannot see any option for Closing the revision ?
Odd I don't see close too, I'll mark it as abandoned, that way it will be marked as closed.