[libcxx-commits] [PATCH] D117368: [libcxx][test] array and basic_string_view iterators are not portably pointers

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

LGTM; none of my remaining nits are blocking.



================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:103-104
 
 template <class Iter, class ValueType, class DiffType, class RefType, class PtrType, class Category>
-constexpr bool testWithoutConcept() {
-  using Traits = std::iterator_traits<Iter>;
-  static_assert(std::same_as<typename Traits::iterator_category, Category>);
-  static_assert(std::same_as<typename Traits::value_type, ValueType>);
-  static_assert(std::same_as<typename Traits::difference_type, DiffType>);
-  static_assert(std::same_as<typename Traits::reference, RefType>);
-  static_assert(std::same_as<typename Traits::pointer, PtrType>);
-  static_assert(!has_iterator_concept_v<Traits>);
-
-  return true;
-}
-
-template <class Iter, class ValueType, class DiffType, class RefType, class PtrType, class Category, class IterConcept>
-constexpr bool testWithConcept() {
+constexpr bool test() {
   using Traits = std::iterator_traits<Iter>;
----------------
FWIW, I'm predictably un-thrilled by the overloading of `test<It, V, C>` and `test<It, V, D, R, P, C>`; I would rather they were named differently, like `testOrdinary` and `testUnusual`, or `test` and `test5`, or something; but I have no really good naming suggestion, and this is still an improvement over the status quo, so no action required unless you feel like it.


================
Comment at: libcxx/test/std/strings/string.view/string.view.deduct/iterator_sentinel.pass.cpp:24
+template<class It, class Sentinel, class CharT>
+constexpr void test_deduction(std::basic_string_view<CharT> val) {
+  auto sv = std::basic_string_view(It(val.data()), Sentinel(It(val.data() + val.size())));
----------------
Consider `s/test_deduction/test_ctad/g` for clarity.


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/advance_to.pass.cpp:42
     context.advance_to(context.begin() + 1);
     assert(std::to_address(context.begin()) == std::to_address(view.begin()) + 1);
 
----------------
Both on lines 29,32 and 42,45, consider spelling the right-hand side of the `==` as just `fmt + 1`.
That is, both `&fmt[1]` and `std::to_address(view.begin()) + 1` are verbose synonyms for `fmt + 1`.

@CaseyCarter, for my own curiosity, would MSVC be happy with
```
assert(context.begin() == view.begin() + 1);
```
or is that considered problematic because of comparing iterators into two different `string_view`s (even though they view the same underlying range)?  ...Hmm, looks like we're doing exactly that on line 33 of `format.parse.ctx/end.pass.cpp`. So maybe it's OK to do it here too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117368