This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges] Implement `views::take`.
ClosedPublic

Authored by var-const on Apr 12 2022, 5:55 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG9924d8d66ae1: [libc++][ranges] Implement `views::take`.
Summary

The view itself has been implemented previously -- this patch only adds
the ability to pipe it.

Also implements P1739 (partially) and LWG3407.

Diff Detail

Event Timeline

var-const created this revision.Apr 12 2022, 5:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2022, 5:55 AM
Herald added a subscriber: mgorny. · View Herald Transcript
var-const requested review of this revision.Apr 12 2022, 5:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2022, 5:55 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const updated this revision to Diff 422200.Apr 12 2022, 5:56 AM

Remove unnecessary change.

philnik added inline comments.
libcxx/include/__fwd/string_view.h
1

__string_view/fwd.h would make more sense I think. Same for span.

17
20–35

Shouldn't string_view be C++17 or later?

libcxx/include/__ranges/take_view.h
234

I would only use the stable names. The numbers bit-rot way to fast.

jloser added a subscriber: jloser.Apr 12 2022, 8:02 AM
jloser added inline comments.
libcxx/include/__fwd/string_view.h
20–35

Technically yes, but for libc++, no. We support string_view in older standards as an extension.

ldionne accepted this revision as: ldionne.Apr 13 2022, 2:33 PM
ldionne added a subscriber: ldionne.

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
1

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
47

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.

226

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.

234

I think I agree -- we're guilty of this in other places and I think that was a mistake, in retrospect.

255–256

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.

ldionne added inline comments.Apr 13 2022, 2:36 PM
libcxx/docs/Status/RangesIssues.csv
15

Note that this is technically not quite complete until we ship ranges::drop.

var-const edited the summary of this revision. (Show Details)Apr 13 2022, 8:38 PM
huixie90 added inline comments.
libcxx/include/__fwd/string_view.h
1

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
191

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

235

I think it should be the different type of _Range&& instead of _Range

259

question : the spec doesn’t say “if the expression is ill formed, goto otherwise clause”. do we need sfinae here?

306

Should the second _Np be _Np&& ?

libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp
51

Could you add a test for

If decltype((F)) does not model convertible_­to<D>, views​::​take(E, F) is ill-formed

philnik added inline comments.Apr 14 2022, 3:59 PM
libcxx/include/__fwd/string_view.h
1

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.

huixie90 added inline comments.Apr 15 2022, 2:48 PM
libcxx/include/__fwd/string_view.h
1

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

var-const updated this revision to Diff 423525.Apr 18 2022, 9:05 PM
var-const marked 11 inline comments as done.

Partially address review feedback and rebase.

In particular, you should look at this comment and make sure we're OK: https://reviews.llvm.org/D116950#inline-1160422.

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

Great point, thanks. Removed for now.

libcxx/include/__fwd/string_view.h
1

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?

17

Thanks!

20–35

string_view is not guarded on the language version.

libcxx/include/__ranges/take_view.h
191

IIUC, this is to prevent potential ODR violations. Added inline, thanks.

226

Yeah, I guess it would have made more sense as a review comment. Removed.

234

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
51

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?)

philnik added inline comments.Apr 19 2022, 10:46 AM
libcxx/include/__fwd/string_view.h
1

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.
So yeah, I do feel quite strongly about this.

11

Just so you don't miss them when moving the headers.

huixie90 added inline comments.Apr 19 2022, 12:40 PM
libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp
51

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>

var-const edited the summary of this revision. (Show Details)Apr 21 2022, 10:36 PM
ldionne accepted this revision.Apr 26 2022, 1:17 PM

This still LGTM, but there are a few comments from other reviewers that need addressing.

libcxx/include/__fwd/string_view.h
1

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.

This revision is now accepted and ready to land.Apr 26 2022, 1:17 PM
var-const updated this revision to Diff 426948.May 4 2022, 2:02 AM
var-const marked 6 inline comments as done.

Address feedback.

libcxx/include/__fwd/string_view.h
1

@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
235

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.

255–256

Done, thanks for calling this out!

259

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.

306

I don't think so. Do you have a case in mind where it would make a difference?

var-const updated this revision to Diff 426954.May 4 2022, 2:29 AM

Fix the CI.

var-const updated this revision to Diff 426958.May 4 2022, 3:27 AM

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.
var-const updated this revision to Diff 427584.May 6 2022, 3:30 AM

Fix the CI.

ldionne accepted this revision.May 6 2022, 9:32 AM
This revision was automatically updated to reflect the committed changes.