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

Wed Dec 1 08:00:19 PST 2021

ldionne requested changes to this revision.
ldionne added a subscriber: tcanens.
ldionne added a comment.
This revision now requires changes to proceed.

Mostly LGTM but I do have a few questions.

================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:28
+
+// [special.mem.concepts]
+
----------------
Quuxplusone wrote:
> var-const wrote:
> > Quuxplusone wrote:
> > > This file is currently named "ranges uninitialized algorithms", but contains no algorithms. I think it should be named `__memory/concepts.h`, for parallelism with `__iterator/concepts.h` and `__ranges/concepts.h`; unless you think it's likely that `<memory>` will gain other unrelated concepts in the near future, in which case maybe we name this `__memory/special_mem_concepts.h` after the name of the subsection.
> > > 
> > > (Alternatively, do I misunderstand the long-term goal here, and this file //will// eventually contain some algorithms?)
> > Right, I should have written a comment about this. The intention is for this file to contain algorithms described in [`specialized.algorithms`](http://eel.is/c++draft/specialized.algorithms). The concepts are exposition-only and specific to these algorithms, which is why I didn't want to create a dedicated file for them. Please let me know if you would still prefer to move them to `__memory/concepts.h` or `__memory/special_mem_concepts.h`.
> Ah, the picture is becoming clearer. Right now `std::uninitialized_default_construct` is defined in `<__memory/uninitialized_algorithms.h>`. You're planning to define the new `std::ranges::uninitialized_default_construct` in `<__memory/ranges_uninitialized_algorithms.h>` ?
> 
> And likewise e.g. `std::ranges::copy` will be defined in `<__algorithm/ranges_copy.h>`, and so on?
> 
> If that's the direction @ldionne prefers, then I'm also okay with it. (Like a year ago, I recall its being unclear whether we were going to make `ranges::copy` live alongside `copy`, etc., and I don't know that a decision was ever made, really. I think the outlined direction makes sense; its only downside is that it will lead to a ton of files named `__algorithm/ranges_*.h`.)
> 
> That said, I'm still weakly in favor of renaming this file to `__memory/concepts.h`. If one day we split up `__memory/ranges_uninitialized_algorithms.h` into `__memory/ranges_uninitialized_copy.h`, `__memory/ranges_uninitialized_default_construct.h`, etc. (which I estimate as fairly likely), it'll be easier if we have the shared `__memory/concepts.h` part already split out.
I don't think we'll want to put `ranges::copy` in the same header as `std::copy`, because those have different sets of dependencies (namely `ranges::copy` needs concepts, while `std::copy` doesn't).

Regarding putting this in `__memory/concepts.h` so that it's already split out if we end up with separate headers for the various uninitialized memory algorithms, I'm neutral. I don't know whether we'll end up with separate headers for those, so I think it's reasonable to either do it right away or to wait until it's needed. I'm fine with either, the cost is minimal.

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

================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.input/input_iterator.compile.pass.cpp:26

-  no_explicit_iter_concept() = default;
-
----------------
Why are we removing those here?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114761