[libcxx-commits] [PATCH] D114761: [libc++][ranges] Implement [special.mem.concepts].

[Tim Song via Phabricator via libcxx-commits]

tcanens added inline comments.


================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:33
+    input_iterator<_Ip> &&
+    // Not a proxy iterator.
+    is_lvalue_reference_v<iter_reference_t<_Ip>> &&
----------------
ldionne wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > var-const wrote:
> > > > Quuxplusone wrote:
> > > > > I recommend deleting this comment line.
> > > > What's the rationale for deleting this comment? If you feel it is incorrect or incomplete, I'd rather improve wording than delete it altogether. It took me some time to figure out the meaning of the additional constraints which I hoped this comment could save for somebody else.
> > > So this comment actually comes from a discussion Costa and I had where we were wondering what this `is_lvalue_reference_v<iter_reference_t<_Ip>> && same_as<remove_cvref_t<iter_reference_t<_Ip>>, iter_value_t<_Ip>>` business was. We came to the common understanding that they were trying to prevent proxy iterators.
> > > 
> > > Does that make sense to you? If so, I think the comment is useful cause it was not entirely obvious to us at first.
> > Mainly the comment line is just confusing because it's so terse: we could rewrite it to `__nothrow_input_iterator accepts input iterators that aren't proxy iterators`, but I'm not sure that's literally true.
> > 
> > I can certainly design something that I'd call a "proxy iterator" that happens to return `T&` from its `operator*` — like, maybe it's like `istream_iterator` and stashes the referred-to thingie inside itself.
> > In fact, `__nothrow_input_iterator<std::istream_iterator<int>> == true` today, right?
> > 
> > I would say that this restriction comes from the fact that we're going to be calling `std::addressof(*it)`, so `*it` needs to be an lvalue: prvalues and xvalues can't be passed to `addressof`.
> > But the most immediate rationale is simply "because we copy-pasted this line directly from the Standard." I don't think we //need// to insert our own speculations. Maybe turn it into a commit-message comment? :)
> FWIW my impression is that those requirements were there so that we know there aren't other potentially throwing operations when one does `iter_reference_t<Iterator> ref = *it`, since that would kind of circumvent the whole point of the concept describing an iterator that doesn't throw. If for example initializing `iter_reference_t<Iterator>` did throw, then you'd end up with a potentially throwing statement in a place where the user probably never expected one, since the concept is `__nothrow_input_iterator`. So my understanding was that when they spec'd it out, they said "let's prevent any subtlety from happening by requiring that these `__nothrow_iput_iterator`s return lvalue references, nothing fancier.
> 
> If the rationale is really just the syntactical requirement of doing `std::addressof(*it)`, then honestly this concept is incredibly mis-named.
> 
> @tcanens Do you know why the following requirements are added to the concept:
> 
> ```
> is_lvalue_reference_v<iter_reference_t<_Ip>> &&
> same_as<remove_cvref_t<iter_reference_t<_Ip>>, iter_value_t<_Ip>>
> ```
> 
> Whatever the answer is, I think it would be useful to record that in a comment, given the discussion we're having right now.
> 
This is for constructing into where `it` points to (or for the input version in particular, used only for `destroy(_n)`, destroying what `it` points to). I don't see how we can implement it if it's not a real reference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114761