[libcxx-commits] [PATCH] D142864: [libc++] `<algorithm>`: `ranges::minmax` should dereference iterators only once

[Igor Zhukov via Phabricator via libcxx-commits]

fsb4000 added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:102
 
-      ranges::minmax_result<_ValueT> __result = {*__first, __result.min};
+      // This initialization is correct, similar to the N4928 [dcl.init.aggr]/6 example
+      ranges::minmax_result<_ValueT> __result = {static_cast<_ValueT>(*__first), __result.min};
----------------
philnik wrote:
> I think this comment is more confusing than helpful TBH.
I changed these comments.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:140
+      // This initialization is correct, similar to the N4928 [dcl.init.aggr]/6 example
+      minmax_result<_ValueT> __result = {static_cast<_ValueT>(*__found_min), __result.min};
+      return __result;
----------------
philnik wrote:
> fsb4000 wrote:
> > fsb4000 wrote:
> > > philnik wrote:
> > > > What are these `static_cast`s for?
> > > MS STL uses the `static_cast`s. I used for consistency with MS STL. I asked them why the static_cast are needed and I added more `static_cast`s here. After their answer and if the `static_cast`s are optionally needed I could remove them.
> > Casey answered me:
> > `indirectly_copyable_storable<_It, _ValueT*>` (https://eel.is/c++draft/alg.req.ind.copy#2) requires that `_ValueT` is `copyable`, but the reference type of the input range (and `_It`) isn't necessarily `const _ValueT&` so we don't have the same guarantee of implicit convertibility. In fact there's no requirement on that algorithm that lets us implicitly convert the result of an iterator dereference to `_ValueT`. There is a requirement that `constructible_from<_ValueT, iter_reference_t<_It>>` holds, however, so we can perform the conversion explicitly with `static_cast`.
> > 
> > https://github.com/microsoft/STL/pull/3384#discussion_r1099671173
> IMO we should add them in another patch then, since this should be checked for all algorithms (most return an iterator, so there shouldn't be many cases) and tested. Could you open an issue for this so we don't forget?
OK


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

https://reviews.llvm.org/D142864