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

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter added inline comments.


================
Comment at: libcxx/include/iterator:532
+concept __can_reference = requires {
+  typename __with_reference<_Tp>;
+};
----------------
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.


================
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&>;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99854