[libcxx-commits] [PATCH] D118687: [libc++][ranges][NFC] Test new requirements for `basic_string_view` and `span` iterators.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.


================
Comment at: libcxx/test/std/containers/views/span.iterators/iterator_concept_conformance.compile.pass.cpp:23
 static_assert(std::contiguous_iterator<iterator>);
+#ifdef _LIBCPP_VERSION
+static_assert(std::__is_cpp17_random_access_iterator<iterator>::value);
----------------
var-const wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > 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?
> > 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.
> (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).
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118687/new/

https://reviews.llvm.org/D118687