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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D114761#3160171 <https://reviews.llvm.org/D114761#3160171>, @Quuxplusone wrote:

> Please split out the test-metaprogramming-refactoring stuff into a separate PR, or abandon it.

I suggest we split it into a separate review, but I'd like us to have that discussion -- I like the composition-of-archetypes approach and IMO it does make several things much easier to understand. It could also lead to greatly reduced duplication in the test suite.



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


================
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.
When Costa showed me this, I actually quite liked it. I thought it was a nice and straightforward way to build archetypes while expressing what they represent concisely, which we have sometimes struggled to do. FWIW, I would probably have suggested writing archetypes that way from the start if I had thought about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114761