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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

Please split out the test-metaprogramming-refactoring stuff into a separate PR, or abandon it.
The actual libcxx/include/ change here looks pretty much fine to me.



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


================
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>> &&
----------------
I recommend deleting this comment line.


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


================
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;
+};
----------------
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.


================
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*>);
----------------
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"));
```


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.input/input_iterator.compile.pass.cpp:110-116
-  bad_iterator_concept() = default;
-
-  bad_iterator_concept(bad_iterator_concept&&) = default;
-  bad_iterator_concept& operator=(bad_iterator_concept&&) = default;
-
-  bad_iterator_concept(bad_iterator_concept const&) = delete;
-  bad_iterator_concept& operator=(bad_iterator_concept const&) = delete;
----------------
Feel free to delete lines 110–116, and remove all the blank lines in this class body. That'll help shorten things up, without metaprogramming.
Ditto lines 89–97 and 69–77 and 50–58.


================
Comment at: libcxx/test/support/test_iterators.h:629-661
+namespace iterator_mixins {
+
+template <typename T>
+struct PreIncrementableOnly {
+  T& operator++();
+};
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114761