[libcxx-commits] [PATCH] D100587: [libc++][ranges] iterator.concept.sizedsentinel: sized_sentinel_for and disable_sized_sentinel_for.

[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 % comments — fix the nits and land it!



================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp:39-41
+  friend constexpr bool operator==(int_sized_sentinel, int*);
+  friend constexpr long operator-(int_sized_sentinel, int*);
+  friend constexpr long operator-(int*, int_sized_sentinel);
----------------
Please remove `constexpr`.
Ditto lines 74-76 and lines 82-86.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp:44-46
+static_assert(
+    !std::sized_sentinel_for<
+        int*, int_sized_sentinel>); // int_sized_sentinel is not an iterator.
----------------
Nit: Either break the line between `;` and `//`, or simply s/int_sized_sentinel/IntSentinel/ so that you don't need to break the line.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp:53
+static_assert(
+    !std::sized_sentinel_for<no_default_ctor, std::vector<int>::iterator>);
+
----------------
`s/std::vector<int>::iterator/int*/`; then you don't need to break the line.
Ditto on line 60.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp:56
+struct not_copyable {
+  not_copyable(not_copyable const&) = delete;
+  bool operator==(std::input_or_output_iterator auto) const;
----------------
Add: `not_copyable() = default;`
Otherwise, this isn't significantly different from the `no_default_ctor` case. (Remember, a class with //any// declared ctors doesn't get a defaulted default ctor, unless you explicitly default the default ctor.)


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp:109-110
+  static_assert(!std::sized_sentinel_for<const_iterator, reverse_iterator>);
+  static_assert(
+      !std::sized_sentinel_for<const_iterator, const_reverse_iterator>);
+
----------------
Remove this linebreak. Ditto through the end of the file.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp:130-131
+static_assert(
+    std::sized_sentinel_for<std::move_iterator<std::vector<int>::iterator>,
+                            std::move_iterator<std::vector<int>::iterator> >);
+
----------------
`s/std::vector<int>::iterator/int*/g` so that you don't have to break the line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100587