Page MenuHomePhabricator

[libc++] [ranges] Implement ranges::cdata
ClosedPublic

Authored by Quuxplusone on Tue, Jan 11, 12:02 PM.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Tue, Jan 11, 12:02 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptTue, Jan 11, 12:02 PM

Can you also update docs/Status/RangesPaper.csv?

libcxx/include/__ranges/data.h
76

Is there any reason to close the ranges namespace above and then reopen it here?

78

I think we agreed to only indent inside __cpo namespaces, so this struct should not have any extra indentation.

84

Question: would decltype(auto) work here?

philnik accepted this revision.Tue, Jan 11, 4:52 PM

LGTM

libcxx/include/__ranges/data.h
83–85

The standard only says static_cast<const T&>(), but that doesn't make much sense, so I'm guessing that's a wording defect?

Quuxplusone marked 3 inline comments as done.Tue, Jan 11, 7:13 PM
Quuxplusone added inline comments.
libcxx/include/__ranges/data.h
76

https://reviews.llvm.org/D89057#inline-1117677
(Also consistency with all the other CPOs at this point, but then, I wrote them ;))

78

True, we did. But I guess I only took that as far as reverting the indentation in uninitialized_foo; I didn't actually go remove the existing indentation in begin/end/cbegin/etc. This was copied from cbegin. I'll go remove that existing indentation tomorrow, unless someone else objects before then (which will certainly give me enough of an excuse to avoid doing it, again ;))

83–85

Serendipitously, Casey Carter emphasized the answer just today in D116991. The standard uses T to refer to the "type" of the expression, which is never a reference type (although it may be cv-qualified). On paper, you can have an lvalue expression of type const int, but you can't have an lvalue expression of type const int&. So when in libc++ we take a parameter of declared type _Tp&&, that corresponds to a paper expression E of "type" remove_reference_t<_Tp>.
TLDR the standard is correct as written; the trick is that its notion of "type T" is not the same as our _Tp here.

84

No, because we need SFINAE.
This is the "you must write it three times" idiom; see Vittorio Romeo's 2017 lightning talk https://www.youtube.com/watch?v=I3T4lePH-yA

var-const accepted this revision.Tue, Jan 11, 7:19 PM

LGTM (modulo the one agreed-upon style nitpick).

libcxx/include/__ranges/data.h
76

Personally, I'd rather have everything in a single contiguous namespace. When I see this reopening, it makes me wonder whether this file contains other nested namespaces (whatever they might be) and leads me to expand the file and start searching around.

84

Thanks for the link!

ldionne accepted this revision.Wed, Jan 12, 9:02 AM

LGTM with passing CI and updated status paper.

Interesting learn about what the Standard means when it says "the type of an expression"!

This revision is now accepted and ready to land.Wed, Jan 12, 9:02 AM
Mordante accepted this revision.Wed, Jan 12, 9:11 AM

LGTM after the CI passes and the status page is updated.

Quuxplusone marked 3 inline comments as done.

Update status page (include a few old PRs too). Rebase to poke CI.

This revision was automatically updated to reflect the committed changes.
var-const added inline comments.Wed, Jan 12, 7:10 PM
libcxx/docs/Status/RangesPaper.csv
100

I think you should also add your name here (so that if anyone is considering taking one of the remaining items in this section, they would know to talk to you).