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

Tue Nov 30 14:49:52 PST 2021

var-const added inline comments.

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

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

================
Comment at: libcxx/test/std/algorithms/specialized.algorithms/special.mem.concepts/special.mem.concepts.compile.pass.cpp:13
+// template<class I>
+// concept __nothrow_input_iterator;
+// template<class S, class I>
----------------
Mordante wrote:
> In general we use one test per concept / function overload. Can you split this test?
Done. It does lead to some duplication, though.

================
Comment at: libcxx/test/std/algorithms/specialized.algorithms/special.mem.concepts/special.mem.concepts.compile.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Quuxplusone wrote:
> Since this file tests non-standard (exposition-only, double-underscored) facilities, it should go in `libcxx/test/libcxx/` instead of `libcxx/test/std/`.
Done, thanks!

================
Comment at: libcxx/test/std/algorithms/specialized.algorithms/special.mem.concepts/special.mem.concepts.compile.pass.cpp:38-42
+template <typename T = int>
+struct InputIterator_Proxy : MoveOnly, Incrementable<InputIterator_Proxy<T>>, HasIteratorConcept<> {
+  using value_type = T;
+  value_type operator*() const;
+};
----------------
Quuxplusone wrote:
> I believe this should be more like https://godbolt.org/z/PTce8axh5 :
> ```
> bool is_nothrow_input_iterator(std::ranges::__nothrow_input_iterator auto) { return true; }
> bool is_nothrow_input_iterator(std::input_iterator auto) { return false; }
> 
> int main() {
>     assert(!is_nothrow_input_iterator(InputIt{}));
>     assert(is_nothrow_input_iterator(NothrowInputIt{}));
> }
> ```
> Also please take the `NothrowInputIt` and `InputIt` from that godbolt; they're pretty minimal.
What's the advantage of using these functions? They add a layer of abstraction and, AFAICT, make this a runtime test rather than a compile-time test. Also, why define a custom `InputIt` when `test_iterators.h` already contains suitable classes?

================
Comment at: libcxx/test/std/algorithms/specialized.algorithms/special.mem.concepts/special.mem.concepts.compile.pass.cpp:49-52
+// __nothrow_sentinel_for
+
+static_assert(std::ranges::__nothrow_sentinel_for<int*, int*>);
+static_assert(!std::ranges::__nothrow_sentinel_for<int*, long*>);
----------------
Quuxplusone wrote:
> For this one, I think it would be reasonable to test that `__nothrow_sentinel_for` subsumes `sentinel_for`, //and// vice versa. However, that might also be overkill. Untested code:
> ```
> bool ntsf_subsumes_sf(__nothrow_sentinel_for<char*> auto) requires true { return true; }
> bool ntsf_subsumes_sf(sentinel_for<char*> auto);
> 
> bool sf_subsumes_ntsf(sentinel_for<char*> auto) requires true { return true; }
> bool sf_subsumes_ntsf(__nothrow_sentinel_for<char*> auto);
> 
> assert(ntsf_subsumes_sf("foo"));
> assert(sf_subsumes_ntsf("foo"));
> ```
Honestly, it seems to me like it's a little overkill. The Standard specifies `nothrow-sentinel-for` to be essentially an alias for `sentinel_for`, and it doesn't really make sense to implement it any other way.

================
Comment at: libcxx/test/support/test_iterators.h:629-661
+namespace iterator_mixins {
+
+template <typename T>
+struct PreIncrementableOnly {
+  T& operator++();
+};
+
----------------
Quuxplusone wrote:
> I strongly, strongly recommend //not// doing this. We need the test code to remain simple, so that when a test fails we can tell quickly why it failed (and whether it's a library bug, as opposed to a bug somewhere deep in someone's test-code metaprogramming).
> 
> Moot: you've also got some bogus default arguments on lines 645 and 651.
I removed this change from the patch. I'll create a separate patch just for this refactoring and we can continue the discussion there.

My primary motivation was to be able to write code that is reasonably close to the way I would describe the problem in natural language. For example, I want to make sure that a move-only, incrementable, dereferenceable class satisfies the input iterator concept -- this is reflected quite closely through a definition like `struct InputIterator : MoveOnly, Incremental<InputIterator>, Readable {`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114761