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

[Mordante]

Summary

Initial patch for implementing https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2520r0.html

[phyBrackets]

Correctly places the macros.

[phyBrackets]

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?

[fsb4000]

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]

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.

[fsb4000]

I've created https://reviews.llvm.org/D142864

[phyBrackets]

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!

[philnik]

Please remove the format-only changes. This makes it incredibly hard to see what you actually changed.

[phyBrackets]

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!

[phyBrackets]

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

[fsb4000]

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.

[phyBrackets]

Thanks @fsb4000

[fsb4000]

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

[phyBrackets]

In D135248#4135268, @fsb4000 wrote:

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

[fsb4000]

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.

[fsb4000]

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

1.txt349 KBDownload

[fsb4000]

libcxx/docs/FeatureTestMacroTable.rst

263

CI failed because extra space there

[fsb4000]

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?

[ldionne]

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

My desire here is to align the various implementations.

[jwakely]

In D135248#4155762, @ldionne wrote:

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

[phyBrackets]

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

[philnik]

LGTM % comments.

libcxx/docs/ReleaseNotes.rst

40–41

libcxx/test/std/iterators/predef.iterators/move.iterators/move.iterator/types.pass.cpp

102

[phyBrackets]

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

[philnik]

In D135248#4161887, @phyBrackets wrote:

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

With the remaining comments address we can land it.

[phyBrackets]

cool, Thanks!

[phyBrackets]

https://github.com/llvm/llvm-project/commit/813e1da9749a5d2d604198cf534f750c842eda22 , commited by me only!

[ldionne]

In D135248#4156007, @jwakely wrote:

In D135248#4155762, @ldionne wrote:

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

[phyBrackets]

Hey @ldionne, Here is the follow up revision https://reviews.llvm.org/D145087

[Mordante]

In D135248#4162128, @phyBrackets wrote:

https://github.com/llvm/llvm-project/commit/813e1da9749a5d2d604198cf534f750c842eda22 , commited by me only!

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?

[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 ?

[Mordante]

In D135248#4162409, @phyBrackets wrote:

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]

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