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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

I also agree with Zoe on the point of testing, but it's been said before so no need for me to. I'm looking forward to the update!



================
Comment at: libcxx/include/CMakeLists.txt:38-41
+  __ranges/begin.h
+  __ranges/cbegin.h
+  __ranges/cend.h
+  __ranges/end.h
----------------
I have to admit this is one instance of a split that, IMO, doesn't add anything. I think it makes a lot more sense to group those into the same header since they are basically always used together (you pretty much always need `end` when you use `begin` and vice versa).

I'd throw that into something like `__ranges/access.h`.


================
Comment at: libcxx/include/__ranges/begin.h:30
+
+namespace ranges {
+  template <class _Tp>
----------------
cjdb wrote:
> zoecarver wrote:
> > 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?
> I think @ldionne wants stuff to be indented inside this namespace, and I don't want to do more than one indentation.
My general rule of thumb is to go for clarity - when I open a namespace and shove a lot of things in it, I tend not to indent everything, but I add a comment at the closing brace. When I open a namespace for just a few things, I generally indent them and skip the closing brace comment since we can easily see what the brace is related to from the indentation. So, no hard rule here, really just do what's most readable.


================
Comment at: libcxx/include/__ranges/begin.h:68
+      else {
+        static_assert(__complete, "`std::ranges::begin` is SFINAE-unfriendly an arrays of an incomplete types.");
+      }
----------------
Typo: `is SFINAE-unfriendly on arrays`

Also, `of an incomplete types` should not be plural.


================
Comment at: libcxx/include/__ranges/end.h:62
+      else {
+        static_assert(__complete, "`std::ranges::end` is SFINAE-unfriendly an arrays of an incomplete types.");
+      }
----------------
Same typos.


================
Comment at: libcxx/include/ranges:20
 namespace std::ranges {
+  inline namespace unspecified {
+    // [range.access], range access
----------------
Is it really relevant to mark those as being part of an unspecified inline namespace in the synopsis?


================
Comment at: libcxx/test/libcxx/ranges/range.access/range.access.begin/incomplete.compile.verify.cpp:29
+  f<incomplete(&)[10]>();
+  // expected-error at __ranges/begin.h:* {{"`std::ranges::begin` is SFINAE-unfriendly an arrays of an incomplete types."}}
+  // expected-error at -2 {{no matching function for call to 'f'}}
----------------
I'd suggest removing the file name here, it's just noise when we move stuff around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100255