[libcxx-commits] [PATCH] D117044: [libc++] [ranges] Implement ranges::cdata

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone marked 3 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/data.h:76
+
+namespace ranges {
+namespace __cdata {
----------------
var-const wrote:
> Is there any reason to close the `ranges` namespace above and then reopen it here?
https://reviews.llvm.org/D89057#inline-1117677
(Also consistency with all the other CPOs at this point, but then, I wrote them ;))


================
Comment at: libcxx/include/__ranges/data.h:78
+namespace __cdata {
+  struct __fn {
+    template <class _Tp>
----------------
var-const wrote:
> I think we agreed to only indent inside `__cpo` namespaces, so this struct should not have any extra indentation.
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 ;))


================
Comment at: libcxx/include/__ranges/data.h:83-85
+      noexcept(noexcept(ranges::data(static_cast<const remove_reference_t<_Tp>&>(__t))))
+      -> decltype(      ranges::data(static_cast<const remove_reference_t<_Tp>&>(__t)))
+      { return          ranges::data(static_cast<const remove_reference_t<_Tp>&>(__t)); }
----------------
philnik wrote:
> The standard only says `static_cast<const T&>()`, but that doesn't make much sense, so I'm guessing that's a wording defect?
Serendipitously, Casey Carter emphasized the answer [just today in D116991](https://reviews.llvm.org/D116991#inline-1119857). 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.


================
Comment at: libcxx/include/__ranges/data.h:84
+      noexcept(noexcept(ranges::data(static_cast<const remove_reference_t<_Tp>&>(__t))))
+      -> decltype(      ranges::data(static_cast<const remove_reference_t<_Tp>&>(__t)))
+      { return          ranges::data(static_cast<const remove_reference_t<_Tp>&>(__t)); }
----------------
var-const wrote:
> Question: would `decltype(auto)` work here?
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117044/new/

https://reviews.llvm.org/D117044