[libcxx-commits] [PATCH] D115315: [libc++][ranges] Implement ranges::uninitialized_default_construct{, _n}.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:39
+
+struct __uninitialized_default_construct_fn final : private __function_like {
+
----------------
ldionne wrote:
> This should be called `__fn` so that it shows up consistently to other CPOs in compiler diagnostics.
Done, thanks. Also added a TODO to change the other few places which don't follow the convention.


================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:37
+
+struct __uninitialized_default_construct_fn final : private __function_like {
+
----------------
Quuxplusone wrote:
> ldionne wrote:
> > var-const wrote:
> > > Quuxplusone wrote:
> > > > This should be as in the blog post too:
> > > > ```
> > > > namespace __uninitialized_default_construct {
> > > > struct __fn {
> > > > };
> > > > } // namespace __uninitialized_default_construct
> > > > ```
> > > > AIUI, this keeps the CPO's own type from ADL'ing into the `std::ranges` namespace; e.g. `foobar(std::ranges::uninitialized_default_construct)` should not consider `std::ranges::foobar` a candidate //even if std::ranges::foobar is not a CPO itself.//
> > > > Also, of course, consistency (Chesterton's Fence, the economist's hundred-dollar bill): if it were safe to omit the namespace, we'd certainly want to do it everywhere, not just here.
> > > Done (I added an `_ns` suffix to the namespace name to make sure it's different from the name of the helper functions). Do we actually need a separate namespace for each CPO, though?
> > > 
> > > By the way, can you please elaborate on how this could be a problem, given that the type of the CPO is an underscore-prefixed internal type?
> > I *think* we could instead have a single namespace named something like `__cpo_detail` and then instead name our helpers `__uninitialized_default_construct_fn`. We could explore that in a separate PR, but for the time being let's be consistent.
> > (I added an `_ns` suffix to the namespace name to make sure it's different from the name of the helper functions).
> 
> (1) please don't add that suffix; (2) what helper functions do you mean? Are you talking about `std::__uninitialized_default_construct`? It's not `std::ranges::__uninitialized_default_construct`, so we don't need to worry about this namespace colliding with it. Again, we have an established convention already and you're being asked to copy it so that we have one convention used everywhere, instead of multiple conventions.
> 
> We //could// (in a separate PR) change libc++'s convention to use names like `std::ranges::__cpo_detail::__uninitialized_default_construct_fn` instead of `std::ranges::__uninitialized_default_construct::__fn`, but that benefits us literally nothing AFAICT and costs us a few bytes per symbol name (which translates to potentially //lots// of .o file size on disk).
> 
> > can you please elaborate on how this could be a problem
> 
> https://godbolt.org/z/4c16P3YGK
Removed the suffix.

> Are you talking about `std::__uninitialized_default_construct`? It's not `std::ranges::__uninitialized_default_construct`, so we don't need to worry about this namespace colliding with it.

Yes, I mean (potential) confusion for the reader, not for the compiler. Changed to follow the convention.

> https://godbolt.org/z/4c16P3YGK

Thanks! When I first thought about it, I presumed that `RandomFunction` would have to be `__RandomFunction` in our case, but now I see that it's not necessarily true.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115315