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

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter marked 3 inline comments as done.
CaseyCarter added inline comments.


================
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>;
----------------
Quuxplusone wrote:
> 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.
I see now that `test<I, V, C>`, `testConst<I, V, C>`, and `testIOterator` are equivalent to calling `test` with different template arguments. I'll rename the first to `testMutable`, and refactor all of them to actually call `test` to eliminate duplication.


================
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);
 
----------------
Quuxplusone wrote:
> 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?
> @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?

For `basic_string_view` we interpret "same underlying sequence" to mean "both iterators were obtained from `basic_string_views` with the same first and last character", so that would be a problem. (And yes, it's a problem in `end`. I have more patches to submit, I've been trying to keep a relatively constant five outstanding reviews.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117368