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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/views_all.h:52
+    constexpr auto operator()(_Tp&& __t) const {
+      return ranges::subrange{_VSTD::forward<_Tp>(__t)};
+    }
----------------
tcanens wrote:
> 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{};`
> I've gotten into the habit of putting `// braces intentional` on anything that genuinely needs brace-init in template code.

I almost approve of that idea! (The problem I see with it is that I think the kind of person who'd put braces when they weren't needed is //also// the kind of person who'd put `// braces intentional` out of a misplaced sense of purpose. ;)) In a perfect world, it'd be useful to have a documented list of "reasons libc++ might use braces," and then the comment could be, like `// braces for reason #2 on that list`.
Here we have a double whammy: "braces intentional(?)" //and// "CTAD intentional(?)".

I would approve even more of a `// CTAD intentional` comment — which wouldn't really need to explain "why," because CTAD does like three things at once and the reason is always "because we want all those things to happen, I guess." Which is also why I'm even more suspicious of unintended CTAD.
https://quuxplusone.github.io/blog/2018/12/09/wctad/

@cjdb, for my own curiosity: Do you think it's feasible for a library to implement C++20 Ranges without ever using CTAD in its implementation? Like, could one realistically spell out the intended template arguments to `subrange` here? or is the CTAD hiding some really hairy and irreducible complexity?  (Leave aside for the moment whether you think it'd be a //good idea// to remove CTAD from libc++'s implementation. Actually my kneejerk assumption is that it would //not// be pragmatic to remove, especially at this early stage, simply because the Standard gives reference implementations in terms of CTAD and so removing our CTAD would simply increase our chances of getting behavior subtly different from the Standard. But in a hypothetical Other Vendor's Implementation, would removing CTAD here and throughout be physically feasible?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102028