This is the initial patch to implement ranges in libc++.
Implements parts of:
- P0896R4 One Ranges Proposal
- P1870 forwarding-range is too subtle
- LWG3379 in several library names is misleading
Differential D90999
[libc++] Implements ranges::enable_borrowed_range Mordante on Nov 7 2020, 1:30 AM. Authored by
Details
This is the initial patch to implement ranges in libc++. Implements parts of:
Diff Detail
Unit Tests Event TimelineComment Actions Don't forget to add the new <ranges> header to std_includes test. Otherwise no other remarks from my side. Comment Actions Added the double_include.sh.cpp test as suggested by @curdeius. Comment Actions I am wondering whether we should split the ranges header up into a smaller part that we use to include in other headers adn the rest. Including all of ranges just for one trait seems a bit of an overkill. We could always add it to concepts as that should be a transitive include for everything that includes range. Thoughts @ldionne?
Comment Actions I agree including the full ranges header in string_view and span feels a bit overkill. But I'll leave the decisions whether or not to split the header with the libc++ maintainers.
Comment Actions Adds the additional unit tests as requested by @miscco. Comment Actions I'd be fine with splitting various parts of ranges into separate headers if that makes the implementation clearer, etc. Comment Actions Move the enable_borrowed_range to a helper header to avoid including <ranges> in <span> and <string_view> as suggested by @miscco. Comment Actions
Comment Actions Overall, LGTM, but I'm wondering if we should hold off on merging till we've got consensus on how detail headers are going to be handled.
Comment Actions AFAICT, the only things are:
If I missed something else from another reviewer, please do it too, and then ship-it. No need for another round of reviews.
Comment Actions Forget about this. I just spoke to Arthur and he explained that the very purpose of splitting the customization point into its own header was to avoid dragging all of <ranges> in the other headers. That makes total sense, I had missed that. So then I think the name of the header should just reflect what's in it, i.e. enable_borrowed_range.h or the like. Still LGTM with that naming sorted out. Comment Actions 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 Actions Remove __ranges/enable_borrowed_range.h from include/module.modulemap. Comment Actions LGTM % comments. I suspect we will end up removing _LIBCPP_INLINE_VISIBILITY in the place it's commented-on, but that can be done in post.
Comment Actions Addresses the final review comments. |
clang-format suggested style edits found: