This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges][NFC] Test new requirements for `basic_string_view` and `span` iterators.
ClosedPublic

Authored by var-const on Feb 1 2022, 2:34 AM.

Details

Summary

Note that most changes to strings and views.span from the One Ranges
Proposal are no longer applicable:

  • free begin and end functions taking basic_string_view and span were removed by P1870;
  • span::const_iterator was removed by LWG3320.

Diff Detail

Event Timeline

var-const requested review of this revision.Feb 1 2022, 2:34 AM
var-const created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2022, 2:34 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added a subscriber: ldionne.Feb 1 2022, 5:38 AM
ldionne added inline comments.
libcxx/test/std/containers/views/span.iterators/iterator_concept_conformance.compile.pass.cpp
23

I don't understand why there is anything to test at all. It seems to me that those sections of the "one ranges proposal" simply don't exist anymore, so there's nothing to do, no?

Quuxplusone accepted this revision.Feb 1 2022, 8:25 AM
Quuxplusone added a subscriber: Quuxplusone.

LGTM % LIBCPP_STATIC_ASSERT.

libcxx/test/std/containers/views/span.iterators/iterator_concept_conformance.compile.pass.cpp
23

Ditto, although this extra coverage doesn't seem harmful.
Please do change it to

LIBCPP_STATIC_ASSERT(std::__is_cpp17_random_access_iterator<iterator>::value);

though (and likewise throughout), both for clarity and because we're trying to cut down on the use of raw #if _LIBCPP_whatever macros in tests.

This revision is now accepted and ready to land.Feb 1 2022, 8:25 AM
var-const added inline comments.Feb 1 2022, 9:56 AM
libcxx/test/std/containers/views/span.iterators/iterator_concept_conformance.compile.pass.cpp
23

(Taking basic_string_view::const_iterator as an example) The text before the <ranges> proposal said that the iterator meets the requirements of a constant random access iterator and [meets the requirements of] a contiguous iterator. After the proposal, this became meets the requirements of Cpp17RandomAccessIterator and models contiguous_iterator (this is still in the latest draft). While it's basically saying the same thing, it formalizes it by using a named requirement.

We were already testing that the iterator types model contiguous_iterator, but not the Cpp17RandomAccessIterator part. If you don't feel this adds value, or if this is already tested elsewhere, I'm very open to dropping the new tests from this patch (so that it becomes just a status update).

var-const updated this revision to Diff 405072.Feb 1 2022, 1:01 PM
  • address feedback;
  • add links to the patch to the status page.
var-const marked an inline comment as done.Feb 1 2022, 1:01 PM
ldionne accepted this revision.Feb 1 2022, 2:47 PM
ldionne added inline comments.
libcxx/test/std/containers/views/span.iterators/iterator_concept_conformance.compile.pass.cpp
23

Thanks for clarifying. I think it does add some value then. Note that __is_cpp17_random_access_iterator doesn't check whether the concept is satisfied, though, it only checks the iterator_category if I'm not mistaken. But still better than nothing.