Page MenuHomePhabricator

[libcxx][ranges] removes default_initializable from weakly_incrementable and view
ClosedPublic

Authored by cjdb on Thu, May 13, 11:22 PM.

Details

Summary

also:

  • removes default constructors from predefined iterators
  • makes span and string_view views

Partially implements P2325.
Partially resolves LWG3326.

Diff Detail

Event Timeline

cjdb requested review of this revision.Thu, May 13, 11:22 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptThu, May 13, 11:22 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.Fri, May 14, 5:15 AM
libcxx/include/span
25–27

Please linebreak for readability:

template<class ElementType, size_t Extent>
  inline constexpr bool ranges::enable_view<span<ElementType, Extent>> =
    (Extent == 0 || Extent == dynamic_extent);
536–537

Ditto here:

template<class _Tp, size_t _Extent>
inline constexpr bool ranges::enable_view<span<_Tp, _Extent>> =
  (_Extent == 0 || _Extent == dynamic_extent);
libcxx/include/string_view
189–191

I would like to see libc++ do the same thing consistently with enable_borrowed_range.h and enable_view.h. I admit, the places that want to specialize enable_view might not save very much (if anything) by avoiding <concepts>, but I think it's worth it to take a consistent approach to these "enable/disable" traits.

This might not be a blocker, since I can fiddle with organization after the fact.

...However, do I read correctly that you are moving ranges::view outside of <__ranges/view.h>? If that was intentional, then probably you should just rename <__ranges/view.h> to <__ranges/enable_view.h>, and that'll fix both complaints.

cjdb updated this revision to Diff 351077.Wed, Jun 9, 11:48 PM

rebases to activate CI

ldionne accepted this revision.Thu, Jun 10, 7:54 AM

What part of the LWG issue are we missing? Is it only the split_view part? If so, I think it's fine to mark it as done since it'll be handled when we implement split_view (http://eel.is/c++draft/range.split.view has the LWG issue applied).

libcxx/test/std/containers/views/range_concept_conformance.compile.pass.cpp
26

Missing space between > and && (elsewhere too).

This revision is now accepted and ready to land.Thu, Jun 10, 7:54 AM
Quuxplusone accepted this revision.Thu, Jun 10, 8:15 AM

LGTM % comments.

libcxx/include/CMakeLists.txt
52–53 ↗(On Diff #351077)

Alphabetize plz

libcxx/include/__ranges/concepts.h
17

Alphabetize plz

libcxx/include/__ranges/enable_view.h
9–11

Update plz

libcxx/include/span
536–537

bump

libcxx/test/std/strings/string.view/range_concept_conformance.compile.pass.cpp
25

Missing space between > and && (elsewhere too).

namely, here

cjdb updated this revision to Diff 351189.Thu, Jun 10, 8:54 AM
cjdb retitled this revision from [libcxx][ranges] makes `basic_string_view` and `span` satisfy concept view to [libcxx][ranges] removes default_initializable from weakly_incrementable and view.
cjdb edited the summary of this revision. (Show Details)

[ http://wg21.link/P2325 | P2325 ] was approved by plenary and made the original version of this patch semi-wrong, so I repurposed the entire patch to deal with P2325 as well.

Due to the excessive changes, existing LGTMs need to be reapproved.

If we're doing p2325 here now, then this patch should include changes to ostream_iterator and so on. (I think it would have been better to ship the LGTM'ed patch and then work on p2325 in a new PR; is that still an option, or is the old code lost now?)

ldionne requested changes to this revision.Thu, Jun 10, 9:24 AM

We'll also need to remember to bump the __cpp_lib_ranges feature-test macro to the right version when we do that (which I assume is when we'll have finished Ranges since we haven't discussed it).

Also, as discussed offline, please make the enable_view.h move in a separate NFC commit to reduce noise.

If we're doing p2325 here now, then this patch should include changes to ostream_iterator and so on. (I think it would have been better to ship the LGTM'ed patch and then work on p2325 in a new PR; is that still an option, or is the old code lost now?)

The patch I'm looking at right now does include the required changes to ostream_iterator, unless you're talking about other changes I'm not seeing?

libcxx/include/iterator
919

Why a FIXME instead of #if _LIBCPP_STD_VER > XXXX?

If both types exist, then they should be the same, so I don't see any sort of ABI issue in case that was the concern.

This revision now requires changes to proceed.Thu, Jun 10, 9:24 AM
Quuxplusone requested changes to this revision.Thu, Jun 10, 9:35 AM

LGTM modulo comments.
(My previous round of comments went in just before the update to include ostream_iterator et al.)

libcxx/docs/Cxx2aStatusPaperStatus.csv
196

@ldionne: What is the process by which papers get added here? Can we get someone to add all the papers that were adopted in this week's plenary? (Then I could check whether D97742 is finally unblocked! ;))

libcxx/include/iterator
919

I agree with Louis's assessment here.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.winc/subsumption.compile.pass.cpp
0

What is this test supposed to be testing?

libcxx/test/std/ranges/range.req/range.view/view.subsumption.compile.pass.cpp
45

This test is now wrong. Please remove lines 55-65, and undo the file move.

cjdb added inline comments.Thu, Jun 10, 10:18 AM
libcxx/include/iterator
919

I want to put this in a more relevant patch.

cjdb added a comment.Thu, Jun 10, 10:25 AM

LGTM modulo comments.
(My previous round of comments went in just before the update to include ostream_iterator et al.)

Not sure what you mean here. I transformed this patch in one step. If there's something I've missed, it's always been missed, so please let me know if that's the case.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.winc/subsumption.compile.pass.cpp
0

It's gone from a subsumption confirmation test to a subsumption regression test, making sure that weakly_incrementable doesn't subsume default_initializable.

libcxx/test/std/ranges/range.req/range.view/view.subsumption.compile.pass.cpp
45

Why is it wrong?

cjdb added inline comments.Thu, Jun 10, 10:51 AM
libcxx/test/std/ranges/range.req/range.view/view.subsumption.compile.pass.cpp
45

Don't worry, I understand now (for future readers: the test is not necessary since the change on libcxx/test/std/ranges/range.req/range.view/view.compile.pass.cpp:49 acts as the regression test). This applies to libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.winc/subsumption.verify.cpp:33 as well.

@Quuxplusone In future, could you please explain why you think something is wrong instead of matter-of-factly stating it? I had no context, and from what I could see, the test looked correct to me.

cjdb updated this revision to Diff 351239.Thu, Jun 10, 12:36 PM

responds to feedback

cjdb updated this revision to Diff 351240.Thu, Jun 10, 12:38 PM

removes extraneous header

cjdb updated this revision to Diff 351242.Thu, Jun 10, 12:42 PM

inconsequential changes

zoecarver accepted this revision as: zoecarver.Thu, Jun 10, 12:55 PM

Let's land this to unblock the other patches.

libcxx/include/iterator
919

Theoretically there's a section in the ranges status document to track things like this... but I guess we don't really use it.

Anyway, please make sure you have this on some todo list that you're going to look at at some point (or put it in the ranges status doc). Make sure it's not just a browser tab ;)

libcxx/include/string_view
662

Why are we putting this change in this patch. I feel like this should be its own patch.

ldionne accepted this revision.Thu, Jun 10, 1:00 PM
ldionne added a subscriber: curdeius.

Thanks! Now we'll be able to update all the in-progress views to this paper.

libcxx/docs/Cxx2aStatusPaperStatus.csv
196

Back in the days, Marshall would keep those lists up to date. More recently, @curdeius has been doing a lot of it. I'll update the list with Monday's plenary.

I missed that un-applied comment in my LGTM a second ago. Still LGTM, but why not fix the FIXME now?

libcxx/include/iterator
919

Actually, I think this should be

#if _LIBCPP_SUPPORTS_RANGES
ranges::iterator_t<Container> iter;
#else
 typename _Container::iterator iter;
#endif

I don't see a reason to wait for another patch?

ldionne added inline comments.Thu, Jun 10, 1:04 PM
libcxx/include/string_view
662

Technically, wg21.link/P2325 does change that specialization as this patch does it. I think it's fine to do both in this patch, even though it could have indeed be tackled as a separate patch that implements only LWG3326. I think it's fine as-is.

Ship-it. I'll be looking forward to a patch that makes insert_iterator up-to-spec for C++20.

libcxx/include/iterator
919

As discussed offline, this will be tackled in a separate patch that makes insert_iterator fully up-to-spec for C++20.

cjdb updated this revision to Diff 351249.Thu, Jun 10, 1:40 PM

rebases for a successful CI

This revision was not accepted when it landed; it landed in state Needs Review.Thu, Jun 10, 3:46 PM
This revision was automatically updated to reflect the committed changes.