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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

Ship it. I suggest going back to add `__voidify` in the places that need it in a separate patch. It should be possible to figure out the places that need it by grepping for `voidify` in the standard.



================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115315