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
194

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
145

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

194

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