[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/uninitialized_algorithms.h:132
     for (; __idx != __last; ++__idx)
-        ::new ((void*)_VSTD::addressof(*__idx)) _Vt;
+        ::new ((void*)_VSTD::addressof(*__idx)) _ValueType;
 #ifndef _LIBCPP_NO_EXCEPTIONS
----------------
ldionne wrote:
> var-const wrote:
> > Quuxplusone wrote:
> > > ldionne wrote:
> > > > Quuxplusone wrote:
> > > > > var-const wrote:
> > > > > > Note: this cast is simpler than the [`voidify`](http://eel.is/c++draft/specialized.algorithms#general-4) function that the Standard mandates. Should I fix this, or is there a reason why a simple cast to `void*` is sufficient here?
> > > > > I can't think of any reason a simple cast to `void*` would be //insufficient//. I'd leave it simple until-and-unless someone comes up with a positive reason and gives us a regression test case. Otherwise, you'll complexify the code and someone six months from now will ask why it can't be simpler.
> > > > When @Quuxplusone originally removed `__voidify` in D93153, I was fine with it. I just did some additional research and I suspect it might have been wrong: https://githubmemory.com/repo/microsoft/STL/issues/1780
> > > > 
> > > > I think we should figure out what the situation is and either confirm that we're conforming, or fix all the places where we use `(void*)x` but the standard says `__voidify(x)`.
> > > (Argh, I keep forgetting to reply to things on the first go-round. Sorry.)
> > > IIUC, STL's comment there indicates that we should add a test case for const iterators, e.g.
> > > ```
> > > auto cit = forward_iterator<const Counted*>(as_array);
> > > auto r = std::ranges::subrange(cit, sentinel_wrapper<forward_iterator<const Counted*>>(cit + 2));
> > > std::ranges::uninitialized_default_construct(r.begin(), r.end());
> > > assert(Counted::current_objects == 2);
> > > assert(Counted::total_objects == 2);
> > > ```
> > > That's missing test coverage today, but AFAIK it's not an argument against the simple cast to `(void*)`. C-style casts are allowed to cast away constness.
> > Added the test.
> > 
> > It looks like until C++20, the Standard defined `voidify` as performing a simple `static_cast<void*>` cast, but that would fail if the given type is cv-qualified because `static_cast` can't cast away cv qualifiers. The new definition works around it by doing two casts, a `static_cast` followed by a `const_cast`. However, it does seem like a simple C-style cast can achieve the same effect. I checked that the C-style cast succeeds when used with const iterators whereas the old `static_cast<void*>` approach fails as expected.
> > 
> > I still feel like we should have a dedicated `__voidify` function (that can use the C-style cast). For one, it would allow defining (and documenting) our approach in one place. It would also follow the way things are laid out in the Standard more closely, which I think is a nice bonus.
> FWIW, I suspect the difference might be that a C-style cast can't appear inside a constexpr context (because it ends up `reinterpret_cast`ing).
> +1 for adding back `__voidify` -- we should probably make this one `_LIBCPP_ALWAYS_INLINE` to improve the debugging experience.
I'm not sure if having a C-style cast affects `constexpr` in this case. From my [experiments](https://wandbox.org/permlink/dEmTAE553hLGKVmf), it looks like the compiler allows C-style casts as long as the cast doesn't actually perform a `reinterpret_cast`:
```
template <typename T>
constexpr void* voidify1(T& from) {
  // This works fine
  return const_cast<void*>(static_cast<const volatile void*>(&from));
}

template <typename T>
constexpr void* voidify2(T& from) {
  // This also works fine, presumably because it ends up
  // doing a `static_cast` followed by a `const_cast`
  return (void*)&from;
}

template <typename T>
constexpr void* voidify3(T& from) {
  struct Bar{};
  Bar* from2 = (Bar*)&from;
  // This doesn't work because converting to `Bar` can only
  // be done via a `reinterpret_cast`
  return (void*)from2;
}
```

I still feel like having a dedicated `__voidify` function. Whichever approach we end up deciding upon, it should be in one place where it's easy to document and easy to change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115315