[libcxx-commits] [PATCH] D93557: [libc++] [P0879] constexpr std::nth_element, and rewrite its tests.

[Jordan Rupprecht via Phabricator via libcxx-commits]

rupprecht added a comment.

Reverted, thanks!

When I was reducing a test case, one thing I noticed was the problem only happens for certain pivot values, e.g.:

  int main() {
    std::vector<int> v = {
        0, 1, 2, 3, 4, 5, 7, 6,
    };
  
    for (int i = 0; i < v.size(); ++i) {
      std::vector<int> v2 = v;
  
      auto pivot = v2.begin() + i;
      std::nth_element(v2.begin(), pivot, v2.end());
  
      for (auto it = v2.begin(); it != pivot; ++it)
        if (*it > *pivot)
          std::cerr << "Bad result with pivot " << i << "\n";
      for (auto it = std::next(pivot); it != v2.end(); ++it)
        if (*pivot > *it)
          std::cerr << "Bad result with pivot " << i << "\n";
    }
  }
  
  $ clang++ /tmp/nth.cpp -stdlib=libc++ && ./a.out
  Bad result with pivot 6
  Bad result with pivot 7

The existing test case operates on `{3,1,4,1,5, 9,2,6,5,3, 5,8,9,7,9}`, which has multiple 1s and multiple 9s, so if the bug is something about incorrectly handling a pivot point at the end or right next to the end (or vice versa for the beginning), then the bug would be masked because the value would be the same in those instances. (e.g. std::nth_element of the last element is 9, but so is std::nth_element of the last - 1 element). So maybe it would be good to also include a test case that has only unique values to avoid that case. (Including a case with equal values is good too, so don't remove that test case).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93557