[libcxx-commits] [PATCH] D120637: [libc++][ranges] Implement ranges::minmax and ranges::minmax_element

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:65
+
+    ranges::minmax_result<_Ip> __result = {*__first, *__first};
+    if (__first == __last || ++__first == __last)
----------------
ldionne wrote:
> philnik wrote:
> > ldionne wrote:
> > > Quuxplusone wrote:
> > > > Per @BRevzin https://cpplang.slack.com/archives/C4Q3A3XB8/p1646101005063689 , I think this needs to be
> > > > ```
> > > > ranges::minmax_result<_Ip> __result = {*first, __result.min};
> > > > ```
> > > > because it might not be safe to copy from `*first` twice. You can regression-test this with something like
> > > > ```
> > > > std::shared_ptr<int> a[] = { std::make_shared<int>(42) };
> > > > auto [min, max] = std::ranges::minmax(std::move_iterator(a), std::move_iterator(a + 1));
> > > > assert(a[0] == nullptr);
> > > > assert(min != nullptr);
> > > > assert(max == min);
> > > > ```
> > > I don't *think* `__result` is considered initialized until the semicolon, though, which means that we'd be accessing an object before it has been initialized, which is UB. Why can't we do this instead:
> > > 
> > > ```
> > > auto __tmp = *__first;
> > > ranges::minmax_result<_Ip> __result = {__tmp, std::move(__tmp)};
> > > ```
> > > 
> > > I asked a question in the thread, waiting for an answer.
> > I think http://eel.is/c++draft/dcl.init.aggr#7 is the line in question here, and that sounds to me like the first element has to be fully initialized before the second one.
> http://eel.is/c++draft/dcl.init.aggr#7 says:
> 
> > The initializations of the elements of the aggregate are evaluated in the element order. That is, all value computations and side effects associated with a given element are sequenced before those of any element that follows it in order.
> 
> I am on the fence about whether this means what you think it means. In particular, if the "initializations of the elements of the aggregate are evaluated in the element order" means that the object isn't considered partially formed before the semi-colon, then the status quo works. I'll ask the question on a reflector, but until we have an answer, I think it would be safer to use this, even if it results in an additional copy:
> 
> ```
> auto __tmp = *__first;
> ranges::minmax_result<_Ip> __result = {__tmp, std::move(__tmp)};
> ```
> 
> Just to unblock this review -- I'll send an email later today.
OK, this seems to be fine to leave as-is per the answers on https://lists.isocpp.org/core/2022/04/12238.php (this is restricted but still useful for those with access -- sorry for others).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637