This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [ranges] Implement move_sentinel and C++20 move_iterator
ClosedPublic

Authored by Quuxplusone on Jan 19 2022, 3:54 AM.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Jan 19 2022, 3:54 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 19 2022, 3:54 AM
Quuxplusone updated this revision to Diff 403488.EditedJan 26 2022, 8:52 PM
Quuxplusone retitled this revision from [libc++] [ranges] move_sentinel and C++20 move_iterator [WIP, needs tests] to [libc++] [ranges] move_sentinel and C++20 move_iterator [WIP, needs more tests].

Added tests for move_sentinel. Still missing: tests for the iterator_traits of move_iterator; tests for three-way comparison; tests for iter_move and iter_swap.

libcxx/include/__iterator/move_iterator.h
197

I found that writing the naïve requires requires { __x.base() == __y.base(); } here did not work; Clang complains something about accessing into incomplete type move_iterator<X> when you go to instantiate it. Pulling the constraint out into a concept outside the body of the class seems to fix the issue.

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 5:51 PM
Quuxplusone retitled this revision from [libc++] [ranges] move_sentinel and C++20 move_iterator [WIP, needs more tests] to [libc++] [ranges] Implement move_sentinel and C++20 move_iterator.
Quuxplusone edited the summary of this revision. (Show Details)
Quuxplusone set the repository for this revision to rG LLVM Github Monorepo.

Rebase, add more tests for comparison operators.
Could still use more tests for

  • iterator_traits
  • iter_move
  • iter_swap

But I think this is pretty much ready to have people start hammering on it.

jloser added inline comments.Mar 2 2022, 7:45 PM
libcxx/include/__iterator/move_iterator.h
99

s/_LIBCPP_CONSTEXPR_AFTER_CXX14/constexpr here and below since we're already guarded with #if _LIBCPP_STD_VER > 17.

197

I'll buy that. I've also run into this several times in other codebases with requires requires expressions in member functions of a class template. It's not great, but the usual "pull it out to a named concept" works.

Rebase, coalesce some repeated ifdefs, use unadorned constexpr in C++20-only codepaths

ldionne accepted this revision.Apr 12 2022, 3:58 PM

This LGTM -- I think we can add a few more tests post-commit, but this looks reasonable as-is too. I'll rebase and ship this.

This revision is now accepted and ready to land.Apr 12 2022, 3:58 PM