[libcxx-commits] [PATCH] D100255: [libcxx] adds `range` access CPOs

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

Not yet finished reviewing, but I've been accumulating comments here over the past few days and want to submit them before stuff moves around too much. I'll have more comments later when I do a proper review.



================
Comment at: libcxx/include/__ranges/end.h:36
+template <class _Tp>
+concept __member_end = __can_borrow<_Tp>&& requires(_Tp&& __t) {
+  typename iterator_t<_Tp>;
----------------
Nit: space.


================
Comment at: libcxx/include/__ranges/end.h:73
+  _LIBCPP_NOEXCEPT_RETURN(
+    end(_VSTD::forward<_Tp>(__t))
+  )
----------------
This should be copied, right?


================
Comment at: libcxx/include/__ranges/end.h:80
+
+inline namespace __cpo {
+inline constexpr auto end = __end::__fn{};
----------------
I haven't quite made it through all the macros/tests yet but can you please make sure there's a test for this where you declare a hidden friend, something like 
```
namespace std { namespace ranges {

struct dummy {
  friend void begin() { }
};

}}
```
that //would// be an ODR violation, but isn't because of this inline namespace? 

In other words, please make sure this fails to compile when the inline namespace is removed. 


================
Comment at: libcxx/include/__ranges/end.h:89
+
+#endif // !defined(_LIBCPP_HAS_NO_RANGES)
+
----------------
I will begrudgingly point out that these should have three underscores. 


================
Comment at: libcxx/include/ranges:10
+
+// #include <compare>              // see [compare.syn]
+// #include <initializer_list>     // see [initializer.list.syn]
----------------
Synopsis usually use a multiline comment. 


================
Comment at: libcxx/test/libcxx/ranges/range.access/access.h:15
+class bad_lvalue_range_unqualified {
+public:
+  int begin(bad_lvalue_range_unqualified&);
----------------
Nit: consider just using structs for all of these. 


================
Comment at: libcxx/test/libcxx/ranges/range.access/access.h:102-103
+
+// An otherwise okay "range", except that it isn't borrowable, and thus can't be accessed as an
+// rvalue
+class rvalue_range_unqualified_not_borrowable {
----------------
Quuxplusone wrote:
> Throughout: please break lines to fit the review window. I suggest breaking after the last `,` (i.e., semantic linebreaks).
Note: the review window is different for everyone. 


================
Comment at: libcxx/test/std/ranges/range.access/array_access.h:30
+  static_assert(!std::invocable<decltype(std::ranges::cpo), double[321]>);                                             \
+  static_assert(!std::invocable<decltype(std::ranges::cpo), long(&)[]>);                                               \
+                                                                                                                       \
----------------
How about rvalue array types?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100255