Page MenuHomePhabricator

[libcxx][iterator] adds `std::ranges::prev`
ClosedPublic

Authored by cjdb on May 15 2021, 6:41 PM.

Details

Summary

Implements part of P0896 'The One Ranges Proposal'.
Implements [range.iter.op.prev].

Depends on D102563.

Diff Detail

Event Timeline

cjdb created this revision.May 15 2021, 6:41 PM
cjdb requested review of this revision.May 15 2021, 6:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2021, 6:41 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Do we need to mark anything down in the status?

libcxx/include/__iterator/prev.h
36

FWIW I think it makes sense to have nodiscard here because it might be confusing to users that think next behaves like advance (advance takes a reference, this one doesn't).

libcxx/include/iterator
492

Synopsis update.

SGTM, but I would like to know what our way forward will be with [[nodiscard]] before approving.

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/prev.pass.cpp
13 ↗(On Diff #345659)

next -> prev

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/prev.verify.cpp
13 ↗(On Diff #345659)

next -> prev

43 ↗(On Diff #345659)

@zoecarver wasn't expected-error@*:* an issue with older versions of Clang?

cjdb added a comment.May 18 2021, 1:42 PM

SGTM, but I would like to know what our way forward will be with [[nodiscard]] before approving.

I think this patch was put up for review after several days of no opposition to my proposal, but before your reply (which I need to reply to).

Quuxplusone added inline comments.
libcxx/include/__iterator/prev.h
36

Yes, _LIBCPP_NODISCARD_EXT plz, being as this is an extension to the Standard. (The Standard doesn't have [[nodiscard]] here: http://eel.is/c++draft/range.iter.op.prev )

@cjdb wrote: "my proposal" — FWIW, I haven't seen anything on the Discord, or the Slack, or libcxx-dev, related to nodiscard. (Maybe I'm not on libcxx-dev? I also sent two emails to libcxx-dev about how debug iterators are totally broken, and never received any replies.) How about you post on the Discord, and/or send an email to the-relevant-people-directly? I would think at least Louis/Zoe/myself/Mark/EricWF. I'll also go poke the Discord right now.

Mordante accepted this revision as: Mordante.May 20 2021, 1:10 PM

LGTM after updating the synopsis, the small comment changes and adjusting the [[nodiscard]] after discussing it on Discord.

cjdb updated this revision to Diff 347843.Tue, May 25, 9:54 PM

Applies @ldionne's feedback (from D102563, since these two patches are siblings):

  • removes [[nodiscard]] from ranges::next (per offline discussion: things not explicitly marked [[nodiscard]] in the wording to be on hold till after P2351R0 and P2377R0 are discussed in Library Evolution)
  • removes [[nodiscard]] from tests
  • splits single next.pass.cpp into four files, one for each overload
ldionne accepted this revision.Wed, May 26, 4:06 PM
This revision is now accepted and ready to land.Wed, May 26, 4:06 PM
cjdb updated this revision to Diff 348143.Wed, May 26, 7:05 PM

rebases to activate CI

This revision was automatically updated to reflect the committed changes.