[libcxx-commits] [PATCH] D100080: [libcxx] adds `std::weakly_incrementable` and `std::incrementable` to <iterator>

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver accepted this revision as: zoecarver.
zoecarver added a comment.

This LGTM sans the small nits.



================
Comment at: libcxx/include/iterator:2579
+
+// [iterator.concept.inc]
+
----------------
I assume this wasn't intentional. 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.inc/incrementable.compile.pass.cpp:201
+static_assert(std::incrementable<std::vector<int>::const_reverse_iterator>);
+static_assert(!std::incrementable<std::vector<int> >);
+} // namespace standard_types
----------------
(Non-blocking.) Does it really make sense to test vector and optional, particularly? It seems pretty obvious that these aren't incrementable. 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.winc/weakly_incrementable.compile.pass.cpp:52
+struct S {};
+static_assert(!std::weakly_incrementable<int S::*>);
+
----------------
You test this exact same thing in the other file. Let's only test it in one place (this file makes more sense to me than the other one). 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.winc/weakly_incrementable.compile.pass.cpp:213
+static_assert(std::weakly_incrementable<
+              std::unordered_multiset<int, int>::const_iterator>);
+
----------------
Same comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100080