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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/__ranges/begin.h:30
+
+namespace ranges {
+  template <class _Tp>
----------------
Nit: You keep opening and closing this namespace but everything in this file goes in it. Why not just open it at the beginning and leave it open?


================
Comment at: libcxx/include/__ranges/begin.h:36
+  template<class _Tp>
+  concept __is_complete = requires { sizeof(_Tp); };
+} // namespace ranges
----------------
Seems like it might be helpful to have this in a more general place, WDYT?


================
Comment at: libcxx/include/__ranges/begin.h:64
+      constexpr bool __complete = __is_complete<remove_cv_t<_Tp> >;
+      if constexpr (__complete) { // used to disable cryptic diagnostic
+        return __t + 0;
----------------
I'm confused, why can't we just do 
```
static_assert(__is_complete<remove_cv_t<_Tp> >, "...");
return __t + 0;
```
?


================
Comment at: libcxx/test/std/ranges/range.access/member_access.h:43
+                                                                                                                       \
+    if (not std::is_constant_evaluated()) {                                                                            \
+      auto r = R(100, typename std::remove_cvref_t<R>::value_type());                                                  \
----------------
Nit: `!std::is_...`


================
Comment at: libcxx/test/std/ranges/range.access/unqualified_lookup_access.h:22
+
+  [[nodiscard]] constexpr friend auto begin(unqualified_lookup_access& x) { return x.data.begin(); }
+
----------------
Maybe make this and the other one `noexcept(false)` with a comment about why. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100255