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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision.
var-const added a comment.

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



================
Comment at: libcxx/include/__ranges/data.h:76
+
+namespace ranges {
+namespace __cdata {
----------------
Quuxplusone wrote:
> 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 ;))
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.


================
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)); }
----------------
Quuxplusone wrote:
> 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
Thanks for the link!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117044