This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges] Finish LWG issues directly related to the One Ranges Proposal.
ClosedPublic

Authored by var-const on May 20 2022, 2:28 AM.

Details

Summary
  • P1252 ("Ranges Design Cleanup") -- deprecate move_iterator::operator-> starting from C++20; add range comparisons to the <functional> synopsis. This restores move_iterator::operator-> that was incorrectly deleted in D117656; it's still defined in the latest draft, see http://eel.is/c++draft/depr.move.iter.elem. Note that changes to *_result types from 6.1 in the paper are no longer relevant now that these types are aliases;
  • P2106 ("Alternative wording for GB315 and GB316") -- add a few *_result types to the synopsis in <algorithm> (some algorithms are not implemented yet and thus some of the proposal still cannot be marked as done);

Also mark already done issues as done (or as nothing to do):

  • P2091 ("Fixing Issues With Range Access CPOs") was already implemented (this patch adds tests for some ill-formed cases);
  • LWG 3247 ("ranges::iter_move should perform ADL-only lookup of iter_move") was already implemented;
  • LWG 3300 ("Non-array ssize overload is underconstrained") doesn't affect the implementation;
  • LWG 3335 ("Resolve C++20 NB comments US 273 and GB 274") was already implemented;
  • LWG 3355 ("The memory algorithms should support move-only input iterators introduced by P1207") was already implemented (except for testing).

Diff Detail

Event Timeline

var-const created this revision.May 20 2022, 2:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2022, 2:28 AM
var-const requested review of this revision.May 20 2022, 2:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2022, 2:28 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik accepted this revision as: philnik.May 20 2022, 2:36 AM
philnik added a subscriber: philnik.

LGTM % nit

libcxx/include/algorithm
669–682

Maybe just move this to the other algorithms?

ldionne accepted this revision.May 24 2022, 11:34 AM
ldionne added a subscriber: ldionne.

LGTM with comments applied and CI passing.

libcxx/include/__iterator/move_iterator.h
161

Can we add a test for this deprecation? Example in libcxx/test/std/utilities/memory/storage.iterator/deprecated.verify.cpp.

libcxx/include/__ranges/empty.h
31 ↗(On Diff #430920)

Do you mind checking-in these formatting changes separately? No need for a review, mark it as NFC.

Doing it as a separate commit makes it less confusing when looking at git blame output, git log and it's also possible to ignore it in .git-blame-ignore-revs (or whatever it's called).

libcxx/include/iterator
409

I would instead say

constexpr pointer operator->() const; // deprecated in C++20

And if they remove it at some point, I would say // deprecated in C++20, removed in C++XY.

libcxx/test/std/ranges/range.access/begin.verify.cpp
13

I think these tests are fine, however it would be nice to add a small comment explaining that we're ensuring that our implementation produces a diagnostic for this ill-formed case.

This revision is now accepted and ready to land.May 24 2022, 11:34 AM
var-const edited the summary of this revision. (Show Details)Jun 27 2022, 7:36 PM
var-const marked 5 inline comments as done.Jun 27 2022, 7:54 PM
var-const added inline comments.
libcxx/include/__iterator/move_iterator.h
161

Done. This uncovered a significant issue -- D117656 incorrectly removed this operator completely in C++20, rather than merely deprecating it. move_iterator::operator-> is still defined (as deprecated) even in the latest draft, in http://eel.is/c++draft/depr.move.iter.elem.

libcxx/include/algorithm
669–682

I'd rather keep as is and do a dedicated cleanup patch for the algorithm synopsis to make sure we follow the ordering from the standard.

var-const updated this revision to Diff 440516.Jun 28 2022, 1:06 AM
var-const marked 2 inline comments as done.

Address feedback

Fix the CI.