Skip to content

Fix excessive comparisons in minmax algorithms#6264

Open
mirion-dev wants to merge 9 commits intomicrosoft:mainfrom
mirion-dev:fix-minmax
Open

Fix excessive comparisons in minmax algorithms#6264
mirion-dev wants to merge 9 commits intomicrosoft:mainfrom
mirion-dev:fix-minmax

Conversation

@mirion-dev
Copy link
Copy Markdown
Contributor

@mirion-dev mirion-dev commented Apr 28, 2026

According to [alg.min.max#32], the algorithms should perform at most max(floor(3/2 (N−1)),0) comparisons, but the current implementations of std::{ranges::}minmax_element violate this requirement when N is even. This can be reproduced with the added test cases in this PR.

Although std::ranges::minmax does not violate it because its upper bound is (3/2) N [alg.min.max#23], I have optimized it as well.

@mirion-dev mirion-dev requested a review from a team as a code owner April 28, 2026 11:40
@mirion-dev
Copy link
Copy Markdown
Contributor Author

It's weird that the failed test P0811R3_midpoint_lerp works on my machine and contains no minmax.

@StephanTLavavej
Copy link
Copy Markdown
Member

@zacklj89 had also encountered lerp(1, 2, 2) == 3; expected 3 on ARM64 internally. I think this test is now flaky and I'll need to separately add workarounds to it.

Comment thread tests/std/tests/Dev11_0532622_minmax_element/test.cpp Outdated
Comment thread tests/std/tests/Dev11_0532622_minmax_element/test.cpp Outdated
Comment thread tests/std/tests/Dev11_0532622_minmax_element/test.cpp Outdated
Comment thread tests/std/tests/Dev11_0532622_minmax_element/test.cpp Outdated
Comment thread tests/std/tests/P0896R4_ranges_alg_minmax/test.cpp Outdated
Comment thread tests/std/tests/Dev11_0532622_minmax_element/test.cpp Outdated
@github-project-automation github-project-automation Bot moved this from Initial Review to Work In Progress in STL Code Reviews Apr 28, 2026
@StephanTLavavej StephanTLavavej requested a review from Copilot April 28, 2026 23:00
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the STL implementations of std::minmax_element / std::ranges::minmax_element to meet the C++ standard comparison-count upper bound for even-sized inputs, and adds targeted tests that reproduce/regress the issue. It also slightly optimizes std::ranges::minmax.

Changes:

  • Adjust std::minmax_element and std::ranges::minmax_element initialization/loop structure to avoid the extra comparison when N is even.
  • Optimize std::ranges::minmax initialization to reduce comparisons.
  • Add new comparison-count regression tests for both legacy and ranges algorithms.
Show a summary per file
File Description
stl/inc/algorithm Fixes minmax-element comparison counts for even N; also optimizes ranges::minmax initialization.
tests/std/tests/P0896R4_ranges_alg_minmax/test.cpp Adds constexpr + runtime comparison-count tests for ranges::minmax and ranges::minmax_element, including an input-range case.
tests/std/tests/Dev11_0532622_minmax_element/test.cpp Adds legacy std::minmax_element comparison-count regression tests (guarded to avoid debug-predicate effects).

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 3/3 changed files
  • Comments generated: 0

Copilot AI review requested due to automatic review settings April 29, 2026 02:44
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/std/tests/Dev11_0532622_minmax_element/test.cpp Outdated
@mirion-dev
Copy link
Copy Markdown
Contributor Author

mirion-dev commented Apr 29, 2026

Failed on P0811R3_midpoint_lerp again.

@StephanTLavavej
Copy link
Copy Markdown
Member

StephanTLavavej commented Apr 29, 2026

Thanks (I will deal with the midpoint lerp failures, somehow).

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Apr 29, 2026
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Initial Review in STL Code Reviews Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: Initial Review

Development

Successfully merging this pull request may close these issues.

4 participants