[libcxx-commits] [PATCH] D102028: [libcxx][ranges] Implement views::all.

[Tim Song via Phabricator via libcxx-commits]

tcanens added inline comments.


================
Comment at: libcxx/include/__ranges/views_all.h:51
+                !requires (_Tp&& __t) { ranges::ref_view{_VSTD::forward<_Tp>(__t)}; })
+    constexpr auto operator()(_Tp&& __t) const {
+      return ranges::subrange{_VSTD::forward<_Tp>(__t)};
----------------
This needs to be SFINAE friendly if the `subrange` construction is ill-formed.


================
Comment at: libcxx/include/__ranges/views_all.h:52
+    constexpr auto operator()(_Tp&& __t) const {
+      return ranges::subrange{_VSTD::forward<_Tp>(__t)};
+    }
----------------
cjdb wrote:
> Quuxplusone wrote:
> > This may be a complete red herring: I vaguely recall an LWG issue and/or paper that pointed out that a whole bunch of brace-initializations in the Standard needed to be parens-initializations, and/or needed to stop using CTAD, because they would do the wrong thing under CTAD. All I can actually find by googling/gmail-searching, though, is @brevzin's https://cplusplus.github.io/LWG/issue3474 . Does anyone know whether (1) such a blanket issue exists, (2) should exist, and/or (3) applies here? @tcanens, thoughts?
> > 
> > (This code looks correct-w.r.t.-the-standard-as-written, modulo `noexcept`.)
> Yes, I think this is informally called Tim-initialisation-fiasco in LWG since he pointed it out. I distinctly remember //something// relevant in Cologne or Belfast, but I thought it was all resolved there. I've gotten into the habit of putting `// braces intentional` on anything that genuinely needs brace-init in template code.
> I vaguely recall an LWG issue and/or paper that pointed out that a whole bunch of brace-initializations in the Standard needed to be parens-initializations, and/or needed to stop using CTAD, because they would do the wrong thing under CTAD.

I think you are thinking about [[ https://cplusplus.github.io/LWG/issue3524 | LWG3524 ]] which will be resolved by [[ https://isocpp.org/files/papers/P2367R0.html | P2367R0 ]]. The other braces in the ranges clause - including these - have no semantic effect and may be replaced with parens at editorial discretion: https://github.com/cplusplus/draft/issues/4593.

> Yes, I think this is informally called Tim-initialisation-fiasco in LWG since he pointed it out. I distinctly remember something relevant in Cologne or Belfast, but I thought it was all resolved there. 

That's a //different// one where the concept currently named `default_initializable` did not require `T{}` to work until [[ https://cplusplus.github.io/LWG/issue3149 | LWG3149 ]] was moved in Belfast, and so all the default member initializers needed to be `T t = T();` instead of `T t{};`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102028