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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:37
+
+struct __uninitialized_default_construct_fn final : private __function_like {
+
----------------
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


================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:32
+
+struct __uninitialized_default_construct_fn final : private __function_like {
+
----------------
var-const wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > I believe this `__function_like` business has caused weird Modules errors before (hence the workarounds you'll see in a few places in the modulemap). It's not needed for conformance, so I would strongly prefer you simply omit it — "Keep It Simple."
> > > Likewise I would personally omit the `final` cruft (it disables the EBO but otherwise has no effect except taking longer to read), but it's not going to cause weird Modules errors, so I don't care as much about whether or not we add `final`. :)
> > About `final` -- we have `[[no_unique_address]]` now so EBO shouldn't be an issue.
> > 
> > Regarding modules issues, basically it means you're going to need to export it from your private submodule, like so:
> > 
> > ```
> > module ranges_uninitialized_algorithms {
> >     private header "__memory/ranges_uninitialized_algorithms.h"
> >     export __function_like
> > }
> > ```
> > 
> > FWIW, I think there is some utility to removing things like addressability from our niebloids, the complexity around modules is just a bit annoying but it's still worth doing. Just my .02, this isn't anything close to "over my dead body".
> I think the idea here is to make sure no users can end up accidentally depending on any properties that aren't explicitly guaranteed by the Standard (and yes, their code would be in error in that case, but it's still a possible problem that would require somebody to spend time fixing it, and we can prevent that problem from happening in the implementation). It does go beyond what is strictly necessary for conformance, but arguably it can result in slightly better experience for the users. Moreover, removing `__function_like` in the future should be a non-breaking change (I don't think ABI is a concern here, though I can easily be wrong on this), whereas going the other way round could realistically break somebody.
> 
> Also, we already have precedent in some of the iterator algorithms (e.g. `advance` and `next`). This isn't a particularly strong argument because there are very few instances, so changing them is quite simple. Basically, we would just need to make sure we are consistent.
(FWIW, I'd still prefer not to expand the sphere of influence of `__function_like` in this PR; but we're all on the same page that it's going to be trivial to remove later. I hope this is also taken as another bit of evidence that having multiple competing conventions sucks, and that consistency saves brain cells.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115315