[libcxx-commits] [PATCH] D90999: [libc++] Implements ranges::enable_borrowed_range

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 13 inline comments as done.
Mordante added a comment.

Thanks for all the reviews. I'll give it one more CI run to see whether the renaming didn't break things. Once that passes I'll land this patch.



================
Comment at: libcxx/include/__ranges_enable_helpers:38
+
+#if _LIBCPP_STD_VER > 17
+
----------------
Mordante wrote:
> cjdb wrote:
> > We've got a `_LIBCPP_HAS_NO_RANGES` macro now that guards on C++20 now.
> Thanks for the information, will adjust this code.
Note I still keep the `_LIBCPP_STD_VER > 17` part, just like we did in `<concepts>`. 


================
Comment at: libcxx/include/string_view:658
+template <class _CharT, class _Traits>
+inline _LIBCPP_INLINE_VISIBILITY constexpr
+bool _VSTD::ranges::enable_borrowed_range<
----------------
ldionne wrote:
> TBH, I haven't thought yet about whether we want `_LIBCPP_INLINE_VISIBILITY` on such inline variables. I guess it's fine if the compilers don't complain (I would have expected GCC to complain cause it expands to `__always_inline__` on it).
This patch passed our CI so it should work with GCC. Since I still have quite some modifications to make I already wanted to give it one more CI run before landing.


================
Comment at: libcxx/test/std/language.support/support.limits/support.limits.general/ranges.version.pass.cpp:10
+// WARNING: This test was generated by generate_feature_test_macro_components.py
+// and should not be edited manually.
+//
----------------
Quuxplusone wrote:
> When adding a new header, I strongly recommend `git grep`-ing for the names of some other recently added headers, e.g. `<charconv>` and `<barrier>` and `<concepts>`, and seeing where they're used, both in `util/` and in `test/`.
> 
> You'll need to modify some of the generators in `util/`, notably the header-inclusion-test generator.
> 
> I can't think of anything else you missed off the top of my head, but grepping will always tell you.
IMO if something is needed for a new header it should be documented in https://libcxx.llvm.org/docs/Contributing.html#adding-a-new-header-todo.
Thanks for catching the missing `python utils/generate_header_inclusion_tests.py;`. That one wasn't done since that script wasn't available when I wrote this patch.


================
Comment at: libcxx/test/std/ranges/range.range/enable_borrowed_range.compile.pass.cpp:42
+  static_assert(!enable_borrowed_range<std::array<int, 0> >);
+  static_assert(!enable_borrowed_range<std::array<int, 1> >);
+  static_assert(!enable_borrowed_range<std::deque<int> >);
----------------
Quuxplusone wrote:
> I recommend using something like `std::array<int, 10>` as your "regular ordinary `std::array`" example (in general, not limited to this specific test). `array<int, 1>` seems like it might get some special cases, especially in Ranges, where there's a lot of obscure logic around the nebulous distinction between "O(1)" and "O(n)".
Good suggestion, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90999