[libcxx-commits] [PATCH] D99854: [libcxx] adds `cpp17-.*iterator` concepts (as `__legacy_.*iterator`)

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked 12 inline comments as done.
cjdb added inline comments.


================
Comment at: libcxx/include/iterator:532
+concept __can_reference = requires {
+  typename __with_reference<_Tp>;
+};
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > CaseyCarter wrote:
> > > ldionne wrote:
> > > > I was surprised to see that we can't simply write `typename _Tp&`. I'm not sure what part of the concepts spec disallows it, do you know?
> > > > 
> > > > Would it make sense to rename this to `__referenceable`? Then below we would have
> > > > 
> > > > ```
> > > > concept __dereferenceable = requires(_Tp& __t) {
> > > >   { *__t } -> __referenceable;
> > > > };
> > > > ```
> > > > 
> > > > which I think is nicely consistent.
> > > > I was surprised to see that we can't simply write `typename _Tp&`. I'm not sure what part of the concepts spec disallows it, do you know?
> > > 
> > > So was I! The _type-requirement_ grammar is "_type-requirement_: `typename` _nested-name-specifier<sub>opt</sub>_ _type-name_ `;`", which doesn't allow any kind of adornments. From a very high level I suppose this makes sense - `_Tp&` is a type, but not the _name_ of a type - but it does mean that some things we expect to work don't.
> > > 
> > > There's probably material here for a 1-2 page paper to propose a change.
> > Re `__can_reference` versus `__referenceable`: This is matching the (current) draft wording in http://eel.is/c++draft/iterator.synopsis#concept:can-reference so I think it's defensible.
> > 
> I agree, `__referenceable` does sound better. But I think it's not outweighed by the benefit of having this code more or less match the standard wording verbatim. 
That name can be editorially changed (probably with LWG's permission), so I'm okay with choosing the name that makes more sense.


================
Comment at: libcxx/include/iterator:541
+template<__dereferenceable _Tp>
+using iter_reference_t = decltype(*declval<_Tp&>());
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
----------------
zoecarver wrote:
> I don't see any tests for `iter_reference_t`. I know it's used in the other implementations so no need to test it heavily, but a simple test file would probably be good. 
Oops, done.


================
Comment at: libcxx/include/iterator:706
+  totally_ordered<_Ip> and
+  requires(_Ip __i, _Ip __j, typename incrementable_traits<_Ip>::difference_type __n) {
+    { __i += __n } -> same_as<_Ip&>;
----------------
CaseyCarter wrote:
> zoecarver wrote:
> > Why do we need `__j` too?
> IIRC one of them should be mutable for the LHS of the assignment operators, and the other `const` for all other uses.
Looks like it's legacy wording now?


================
Comment at: libcxx/test/libcxx/iterators/iterator.requirements/iterator.assoc.types/iterator.traits/legacy_bidirectional_iterator.compile.pass.cpp:159
+static_assert(!std::__legacy_bidirectional_iterator<
+              std::unordered_set<int, int>::iterator>);
+static_assert(!std::__legacy_bidirectional_iterator<
----------------
zoecarver wrote:
> This should only have one template. Otherwise it's going to think `int` is the hash type ;)
Done, everywhere :-)


================
Comment at: libcxx/test/libcxx/iterators/iterator.requirements/iterator.assoc.types/iterator.traits/legacy_forward_iterator.compile.pass.cpp:70
+static_assert(!std::__legacy_forward_iterator<
+              std::back_insert_iterator<std::vector<int> > >);
+static_assert(!std::__legacy_forward_iterator<
----------------
zoecarver wrote:
> Why isn't this a __legacy_forward_iterator? 
Diagnostics report that it's not `equality_comparable`.


================
Comment at: libcxx/test/libcxx/iterators/iterator.requirements/iterator.assoc.types/iterator.traits/legacy_forward_iterator.compile.pass.cpp:151
+static_assert(std::__legacy_forward_iterator<
+              std::unordered_set<int, int>::const_iterator>);
+
----------------
zoecarver wrote:
> Same comment. And for `unordered_multiset`. Also why not test (const_) local_iterator for both while you're at it?
TIL about `local_iterator`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99854