This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Jan 11 2022, 12:02 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 11 2022, 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.Jan 11 2022, 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.Jan 11 2022, 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.Jan 11 2022, 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.Jan 12 2022, 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.Jan 12 2022, 9:02 AM
Mordante accepted this revision.Jan 12 2022, 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.Jan 12 2022, 7:10 PM
libcxx/docs/Status/RangesPaper.csv
100 ↗(On Diff #399529)

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