[libcxx-commits] [PATCH] D100160: [libcxx] adds `std::input_or_output_iterator` and `std::sentinel_for` to <iterator>

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general LGTM modulo some nits and the request of @miscco. But I like to know more about the locale_dependent test before approving.



================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sentinel_for.compile.pass.cpp:13
+
+// template<class In>
+// concept sentinel_for;
----------------
Please update this part to `template<class S, class I>`


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sentinel_for.compile.pass.cpp:102
+  static_assert(!std::sentinel_for<const_iterator, reverse_iterator>);
+  static_assert(!std::sentinel_for<const_iterator, const_reverse_iterator>);
+
----------------
Here `iterator` and `const_iterator` are not tested against the full set, like `reverse_iterator` and `const_reverse_iterator`. Can you add them for completeness?


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/locale_dependent.compile.pass.cpp:12
+// UNSUPPORTED: gcc-10
+// REQUIRES: locale.en_US.UTF-8
+
----------------
What's locale dependent with this test?
If it's locale dependent it should contain a line `// UNSUPPORTED: libcpp-has-no-localization`


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/locale_dependent.compile.pass.cpp:21
+// template<class S, class I>
+// concept sized_sentinel_for;
+
----------------
Please remove the `sized_sentinel` since it isn't added here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100160