Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rG9924d8d66ae1: [libc++][ranges] Implement `views::take`.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/include/__fwd/string_view.h | ||
---|---|---|
21–36 | Technically yes, but for libc++, no. We support string_view in older standards as an extension. |
You might also want to take a look at D115122 for inspiration if you have not already -- just leaving this note since I stumbled upon a comment that suggested doing that on my ranges::drop review. In particular, you should look at this comment and make sure we're OK: https://reviews.llvm.org/D116950#inline-1160422.
Since I'm about to go OOO and I don't see any real blocking issues, I'm going to approve this -- I don't think I'd need to see this again after CI is passing and my comments have been addressed.
libcxx/include/__fwd/string_view.h | ||
---|---|---|
2 | FWIW I don't really have a strong opinion. The only downside to this is that we might end up with a lot of one-header subdirectories if we start adding forward declarations to more classes, and putting everything under __fwd/ would avoid that. | |
libcxx/include/__ranges/take_view.h | ||
49–50 | Can you land the indentation change as a NFC patch before this one? No need for a review, I'd just like to avoid mixing it with this patch. | |
258 | I don't think this comment is necessary to keep around in the long run. I understand its purpose in the context of this review, however. | |
266 | I think I agree -- we're guilty of this in other places and I think that was a mistake, in retrospect. | |
287–288 | It is not clear to me that we want to use std::forward here. Indeed, http://eel.is/c++draft/range.take#view does say U(ranges::begin(E), ranges::begin(E) + std::min<D>(ranges::distance(E), F)) where E is the expression in views::take(E, F). However, we generally want to avoid forwarding in more than one expression at a time because that sets us up for double-moves. In this case it should not matter since begin has to take either T& or T const&, but I think I'd drop the forward here. IIUC, the same is true for ranges::distance. |
libcxx/docs/Status/RangesIssues.csv | ||
---|---|---|
15 ↗ | (On Diff #422200) | Note that this is technically not quite complete until we ship ranges::drop. |
libcxx/include/__fwd/string_view.h | ||
---|---|---|
2 | I am in favour of fwd folder. I don’t like to have too many files with the exact same name fwd.hpp | |
libcxx/include/__ranges/take_view.h | ||
223 | I remember that clangd always suggests me to add inline in those partial variable template specialisations. I don’t get why but those constexpr bool variable template in the standard all have inlines | |
267 | I think it should be the different type of _Range&& instead of _Range | |
291 | question : the spec doesn’t say “if the expression is ill formed, goto otherwise clause”. do we need sfinae here? | |
338 | Should the second _Np be _Np&& ? | |
libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp | ||
52 | Could you add a test for
|
libcxx/include/__fwd/string_view.h | ||
---|---|---|
2 | I don't like having a __fwd subdirectory because that would mean we have a single header spread across multiple subdirecories. That makes it a lot harder to see what is part of a header. |
libcxx/include/__fwd/string_view.h | ||
---|---|---|
2 | I think having a single fwd folder whose subdirectories hierarchies follow the non-fwd hierarchies is pretty neat. You can easily find the forward declarations by adding a fwd at the beginning. One example code base is Boost.Hana |
There is a test case for a fixed-size span:
// `views::take(span, n)` returns a `span` with a dynamic extent, regardless of the input `span`. { std::span<int, 8> s(buf); std::same_as<std::span<int, std::dynamic_extent>> decltype(auto) result = s | std::views::take(3); assert(result.size() == 3); }
libcxx/docs/Status/RangesIssues.csv | ||
---|---|---|
15 ↗ | (On Diff #422200) | Great point, thanks. Removed for now. |
libcxx/include/__fwd/string_view.h | ||
2 | I'm slightly in favor of having all the forward declarations under a single folder because it would make it easier to see what can and cannot be forward-declared. While having contents of a single header spread across several directories in general would be bad, I think splitting out forward declarations following a consistent pattern shouldn't cause any issues. @philnik How strongly do you feel about this? | |
18 | Thanks! | |
21–36 | string_view is not guarded on the language version. | |
libcxx/include/__ranges/take_view.h | ||
223 | IIUC, this is to prevent potential ODR violations. Added inline, thanks. | |
258 | Yeah, I guess it would have made more sense as a review comment. Removed. | |
266 | Thanks, this is a good point, but there doesn't seem to be a great alternative. Added short descriptions instead. | |
libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp | ||
52 | This is checked by static_assert(!CanBePiped<SomeView&, decltype(std::views::take(/*n=*/NotAView{}))>); Would you prefer a more tailored test? (e.g. rather than having an empty type, come up with a more plausible number type that isn't convertible?) |
libcxx/include/__fwd/string_view.h | ||
---|---|---|
2 | The reason you want to forward declare the things here is to avoid including all of the header and having longer compile times, right? If there is another technical reason for that, please tell me! So there shouldn't be any reason for new headers to have them in 2-3 years (hopefully). I don't think there will be a lot of new forward declared headers because of this, so I think having forward declarations in another directory is quite surprising. | |
11 | Just so you don't miss them when moving the headers. |
libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp | ||
---|---|---|
52 | ah. i didn't spot this particular line. I was looking at the lines that are checking is_invocable_v<decltype(views::take), ...> and didn't see something like is_invocable_v<decltype(views::take), SomeView, NotADifference> |
This still LGTM, but there are a few comments from other reviewers that need addressing.
libcxx/include/__fwd/string_view.h | ||
---|---|---|
2 | The main reason for having forward declarations is to avoid circular dependencies. For example, <span> includes some headers from <__ranges/...>, and it's possible that some of them also require including <span>. Having an easy way to add forward declarations is useful for that. I don't want us to block this patch on such a simple issue. Let's add forward declarations in __fwd/FOO.h from here on, and we can always change that (or even try to remove as many forward declarations as possible) in the future, if there is a good reason to do so. |
Address feedback.
libcxx/include/__fwd/string_view.h | ||
---|---|---|
2 | @philnik Like Louis said, having some forward declarations is probably unavoidable. But even if we were to remove them in the future, it seems like it would still be (very slightly) easier if they're all within a single folder. In any case, changing this structure in either direction is very straightforward, so we can always change our solution later on with minimal hassle. I'm inclined to keep as is -- if you still feel strongly, I can do a follow-up (feel free to ping me on Discord if that's the case). | |
11 | Thanks a lot! | |
libcxx/include/__ranges/take_view.h | ||
267 | In this case, I don't think it matters. range_difference_t<T> depends on iterator_t<T>, which in turn is defined as decltype(ranges::begin(std::declval<T&>())). I think the type of T& would be the same regardless of whether T is _Range or _Range&&. It's a good question in general, though -- please let me know if you see a similar pattern where this could make a difference. | |
287–288 | Done, thanks for calling this out! | |
291 | The way I interpret it is that it says "If the type satisfies such and such condition, do this, otherwise go to the next clause". If the resulting expression is ill-formed, then the type does not satisfy the condition, so it should check the next clause. Sorry if I misunderstood the question -- in that case, please elaborate a little. | |
338 | I don't think so. Do you have a case in mind where it would make a difference? |
Fix the CI:
- appease clang-tidy;
- make sure to undefine min/max macros in the forward-declaration headers;
- tweak the modulemap linter to recognize forward-declaration headers.
clang-format not found in user’s local PATH; not linting file.