Page MenuHomePhabricator

[libc++] implement move_iterator<T*> should be a random access iterator
AbandonedPublic

Authored by Mordante on Oct 5 2022, 1:33 AM.

Diff Detail

Unit TestsFailed

TimeTest
4,240 mslibcxx 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

There are a very large number of changes, so older changes are hidden. Show Older Changes

Correctly places the macros.

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
fsb4000 added a comment.EditedJan 29 2023, 9:02 PM

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.

fsb4000 added a comment.EditedFeb 17 2023, 8:19 AM

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

Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2023, 10:04 AM

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

Thanks @fsb4000 , missed the main

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...

fsb4000 added inline comments.Sun, Feb 26, 8:48 AM
libcxx/docs/FeatureTestMacroTable.rst
263

CI failed because extra space there

phyBrackets marked an inline comment as done.
fsb4000 accepted this revision.Sun, Feb 26, 3:53 PM

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.

@jwakely Does libstdc++ implement P2520R0 in C++20 as a DR too?

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.

Hey @fsb4000 @ldionne , Let me know if it's fine to commit this. I'll do it. Thanks!

philnik accepted this revision.Wed, Mar 1, 8:21 AM

LGTM % comments.

libcxx/docs/ReleaseNotes.rst
40–41
libcxx/test/std/iterators/predef.iterators/move.iterators/move.iterator/types.pass.cpp
102

Hey @philnik , I'm not sure about, can we land this now?

Hey @philnik , I'm not sure about, can we land this now?

With the remaining comments address we can land it.

cool, Thanks!

@jwakely Does libstdc++ implement P2520R0 in C++20 as a DR too?

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 ?

Mordante commandeered this revision.Wed, Mar 1, 12:30 PM
Mordante added a reviewer: phyBrackets.

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'll try it by taking over the patch.

Mordante abandoned this revision.Wed, Mar 1, 12:32 PM

Odd I don't see close too, I'll mark it as abandoned, that way it will be marked as closed.