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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:85-86
+                    is_rvalue_reference_v<range_reference_t<_Range>>) {
+        auto __test_for_one_element = __first;
+        if (++__test_for_one_element == __last) {
+          // This initialization is correct, similar to the N4928 [dcl.init.aggr]/6 example
----------------



================
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};
----------------
I think this comment is more confusing than helpful TBH.


================
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;
----------------
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?


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

https://reviews.llvm.org/D142864