This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][ranges] implements dangling, borrowed_iterator_t, borrowed_subrange_t
ClosedPublic

Authored by cjdb on Jun 30 2021, 9:39 AM.

Details

Summary

Diff Detail

Event Timeline

cjdb created this revision.Jun 30 2021, 9:39 AM
cjdb requested review of this revision.Jun 30 2021, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2021, 9:39 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 355600.Jun 30 2021, 9:41 AM

removes stray include

tcanens added inline comments.
libcxx/include/__ranges/borrowed_subrange_t.h
37–38 ↗(On Diff #355600)

I'm under the impression that libc++ has a "better conditional_t" somewhere that doesn't require one class template instantiation per type?

ldionne added inline comments.Jun 30 2021, 2:22 PM
libcxx/include/__ranges/borrowed_subrange_t.h
37–38 ↗(On Diff #355600)

You can either use _If or just plain conditional_t. _If creates fewer instantiations.

LGTM mod comments!

libcxx/include/__ranges/borrowed_subrange_t.h
37–38 ↗(On Diff #355600)

Yes, and I think it's safe to use here because _Rp is already constrained to be a range, so subrange<iterator_t<_Rp>> is guaranteed well-formed. (If _Rp were allowed to be a non-range, then we couldn't use this trick: godbolt)

template<range _Rp>
using borrowed_subrange_t = _If<borrowed_range<_Rp>, subrange<iterator_t<_Rp>>, dangling>;
libcxx/include/__ranges/dangling.h
35–42
template<range _Rp>
using borrowed_subrange_t = _If<borrowed_range<_Rp>, iterator_t<_Rp>, dangling>;
libcxx/include/__ranges/subrange.h
20–21

Yikes. I guess I need to audit graph_header_deps.py; it should have caught even this trivially circular inclusion.

libcxx/include/ranges
141

a-z plz (and also in the modulemap and CMakeLists)

libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp
31

As long as you're using same_as instead of is_same_v, you might as well also use concept instead of constexpr bool — it's shorter (and this line coincidentally is 2 characters too long :)).

libcxx/test/std/ranges/range.utility/range.dangling/borrowed_subrange.compile.pass.cpp
24

IMO it's worth testing std::ranges::borrowed_subrange_t<std::string&> and std::ranges::borrowed_subrange_t<std::string&&>.

Using string and string_view as your archetypes for "borrowed" and "non-borrowed" seems like a good idea to me. Vice versa, assuming that we've already got a good test verifying that std::ranges::borrowed_range<std::span<T>> in the test suite for span, I think it'd be fine to eliminate all the lines here involving <vector> and <span>: they're too redundantly similar to the string/string_view lines.

libcxx/test/std/ranges/range.utility/range.dangling/dangling.compile.pass.cpp
27–28

I'd lose lines 25-27 (as redundant) but add

static_assert(std::is_nothrow_constructible_v<std::ranges::dangling, int, int, int>);
static_assert(std::is_trivial_v<std::ranges::dangling>);

assuming that those properties are in fact intentional and/or mandated.

cjdb updated this revision to Diff 355771.Jun 30 2021, 9:23 PM
cjdb marked 4 inline comments as done.
  • replaces specialisations with _If
  • adds tests
  • moves borrowed_subrange_t into __range/subrange.h (ATTN @ldionne)
cjdb updated this revision to Diff 355776.Jun 30 2021, 10:16 PM

fixes a few things

zoecarver accepted this revision as: zoecarver.Jul 1 2021, 10:11 AM

A few nits. But LGTM.

libcxx/include/ranges
98

Thanks.

101

Is this implemented? It doesn't look like it, yet.

libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp
33

While you're at it (changing to a concept) can you rename this to something more descriptive? Maybe something like "hasIterator" or "validForBorrowedIter" or something that indicates you're checking the requirements (that T is a range).

cjdb added inline comments.Jul 1 2021, 12:22 PM
libcxx/include/__ranges/borrowed_subrange_t.h
37–38 ↗(On Diff #355600)

Fewer instantiations is always good.

libcxx/include/ranges
101

This was the first thing implemented in <ranges> (see line 136).

141

Done, done, and done!

libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp
31

has_X is a trait, not a concept :)

libcxx/test/std/ranges/range.utility/range.dangling/borrowed_subrange.compile.pass.cpp
24

IMO it's worth testing std::ranges::borrowed_subrange_t<std::string&> and std::ranges::borrowed_subrange_t<std::string&&>.

Yes, good idea (also applied to borrowed_iterator).

Using string and string_view as your archetypes for "borrowed" and "non-borrowed" seems like a good idea to me. Vice versa, assuming that we've already got a good test verifying that std::ranges::borrowed_range<std::span<T>> in the test suite for span, I think it'd be fine to eliminate all the lines here involving <vector> and <span>: they're too redundantly similar to the string/string_view lines.

I'd still prefer to keep the other types to make sure borrowed_X isn't hard-coded to a single type.

libcxx/test/std/ranges/range.utility/range.dangling/dangling.compile.pass.cpp
27–28

I'd lose lines 25-27 (as redundant) but add

static_assert(std::is_nothrow_constructible_v<std::ranges::dangling, int, int, int>);
static_assert(std::is_trivial_v<std::ranges::dangling>);

Keeping lines 25-7 (making sure this works with user-defined types is important), and added both your lines.

assuming that those properties are in fact intentional and/or mandated.

🤷 The constructor is variadic, so it's probably intentional and mandated?

cjdb updated this revision to Diff 356567.Jul 5 2021, 2:54 PM

removes is_trivial test, since clang and gcc don't agree

ldionne requested changes to this revision.Jul 8 2021, 1:45 PM
ldionne added inline comments.
libcxx/include/__ranges/dangling.h
33

_LIBCPP_HIDE_FROM_ABI

libcxx/include/__ranges/subrange.h
22

You need <type_traits> for _If (here and in dangling.h).

libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp
33

Agreed, I would just use

template <class T>
concept has_borrowed_iterator = requires {
  typename std::ranges::borrowed_iterator_t<T>;
};

That's closest to what we do in other tests, no?

libcxx/test/std/ranges/range.utility/range.dangling/dangling.compile.pass.cpp
27–28

Instead, why not:

template<int> struct S { };
static_assert(std::is_nothrow_constructible_v<std::ranges::dangling>);
static_assert(std::is_nothrow_constructible_v<std::ranges::dangling, S<0>>);
static_assert(std::is_nothrow_constructible_v<std::ranges::dangling, S<0>, S<1>>);
static_assert(std::is_nothrow_constructible_v<std::ranges::dangling, S<0>, S<1>, S<2>>);

Feel free to keep (or not) the int, int* and int (*)() ones, I'm not sure how much coverage they add. IMO using S<n> conveys more clearly that we're making sure that dangling can be constructed from something arbitrary.

This revision now requires changes to proceed.Jul 8 2021, 1:45 PM
cjdb updated this revision to Diff 357359.Jul 8 2021, 2:15 PM
cjdb marked 11 inline comments as done.

applies requested changes

libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp
33

Renamed, but keeping as constexpr bool (this is not a concept).

ldionne requested changes to this revision.Jul 9 2021, 7:16 AM
ldionne added a subscriber: DavidSpickett.

@DavidSpickett I see the Linaro bots have started failing when building libunwind. I looked into it a little bit, but I didn't see any recent changes in libunwind. Did you make changes to your bots?

libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp
39–43

I think this belongs to dangling.compile.pass.cpp.

libcxx/test/std/ranges/range.utility/range.dangling/dangling.compile.pass.cpp
2

This should be a .pass.cpp test, and we should test that we can actually run (not only compile) code that calls the variadic constructor (and the default constructor), since it's defined with an empty body in the Standard. As it stands, I think our tests would pass if we had not defined the constructor at all and only declared it, but we would be non-conforming.

This revision now requires changes to proceed.Jul 9 2021, 7:16 AM
cjdb updated this revision to Diff 358073.Jul 12 2021, 2:45 PM

rebases to use the new shiny CI

ldionne added inline comments.Jul 12 2021, 8:57 PM
libcxx/test/std/ranges/range.utility/range.dangling/borrowed_iterator.compile.pass.cpp
39–43

I think this belongs to dangling.compile.pass.cpp.

This is where this comment belongs (for some reason it doesn't show up).

cjdb updated this revision to Diff 358143.Jul 12 2021, 9:59 PM

applies @ldionne's feedback (in some fashion)

cjdb added inline comments.Jul 12 2021, 10:02 PM
libcxx/test/std/ranges/range.utility/range.dangling/dangling.compile.pass.cpp
2

Does it really need to be a .pass.cpp? Surely a bunch of constexpr objects should do the trick, no?

cjdb updated this revision to Diff 358146.Jul 12 2021, 10:05 PM

fixes mistake

ldionne added inline comments.Jul 14 2021, 9:54 AM
libcxx/test/std/ranges/range.utility/range.dangling/dangling.compile.pass.cpp
2

We really want to test exactly what the users are going to do, and that includes "running" that empty constructor at runtime. Even though in practice we do know it will be optimized out and all, it's still what we need to check.

cjdb updated this revision to Diff 359365.Jul 16 2021, 9:54 AM

Applies @ldionne's feedback about adding a run-time test.

Thought I already did this, but apparently not. Explains why there hasn't been any further progress on this.

ldionne accepted this revision.Jul 16 2021, 10:24 AM
This revision is now accepted and ready to land.Jul 16 2021, 10:24 AM
cjdb updated this revision to Diff 359475.Jul 16 2021, 4:19 PM

rebasing to activate CI