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

Tue Nov 30 17:01:03 PST 2021

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

LGTM mod three extremely nitty style nits — and ideally renaming to `__memory/concepts.h`, unless @ldionne disagrees, in which case OK.

IIUC your changes to `input_iterator.compile.pass.cpp` are now entirely NFC-cleanup and can be committed separately from the functional part. (No additional Phab review needed AFAIC; I'd say just land it in its own commit.)

================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:28
+
+// [special.mem.concepts]
+
----------------
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.

================
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:
> 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? :)

================
Comment at: libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_iterator.compile.pass.cpp:20
+
+struct ForwardIterator_Proxy {
+  using value_type = int;
----------------
Naming nit: `ForwardProxyIterator` (or perhaps `ProxyForwardIterator` but I like that worse).
Ditto throughout.

================
Comment at: libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_iterator.compile.pass.cpp:25-27
+  bool operator==(const ForwardIterator_Proxy&) const;
+
+  int operator*() const;
----------------
Another trivial nit here: I'd delete blank line 26 and swap the order of lines 25 and 27.

================
Comment at: libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp:20
+
+// Has to be a template to work with `test_range`.
+template <typename>
----------------
...and this is why `test_range` sucks. ;) But OK for now. I have a long-self-assigned TODO to clean up some of this stuff.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114761