[libcxx-commits] [PATCH] D93512: [libc++] [P0879] constexpr heap and partial_sort algorithms

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/make.heap/make_heap.pass.cpp:26
 {
-    int* ia = new int [N];
-    for (int i = 0; i < N; ++i)
-        ia[i] = i;
-    std::shuffle(ia, ia+N, randomness);
-    std::make_heap(ia, ia+N);
-    assert(std::is_heap(ia, ia+N));
-
-    typedef random_access_iterator<int *> RI;
-    std::shuffle(RI(ia), RI(ia+N), randomness);
-    std::make_heap(RI(ia), RI(ia+N));
-    assert(std::is_heap(RI(ia), RI(ia+N)));
-
-    delete [] ia;
+    int orig[15] = {3,1,4,1,5, 9,2,6,5,3, 5,8,9,7,9};
+    T work[15] = {3,1,4,1,5, 9,2,6,5,3, 5,8,9,7,9};
----------------
ldionne wrote:
> Can you please also add a few dumb hand-written test cases? You know I love those, right?
> 
> Same for the other algorithms whose tests have been rewritten.
The trick for `make_heap` is that there are multiple possible outputs (theoretically): heapifying `{1,2,3}` might give you `{3,1,2}` or it might give you `{3,2,1}`. Both possibilities are equally conforming, even though I would guess that all implementations really do the same thing in practice.

How much do we care about that? Do we mind hard-coding one of the outputs as "blessed"?

Anyway, I'm resisting out of laziness — couldn't someone else add ad-hoc tests in a separate commit? — but maybe it'd help to see an example of the kind of test you're thinking of. I mean would it just be like this?

```
TEST_CONSTEXPR_CXX20 bool test_simple() {
    int a[] = {5,2,4,1,3};
    std::push_heap(a, a+5);
    int expected[] = {5,3,4,1,2};
    assert(std::equal(a, a+5, expected, expected+5));
    return true;
}

TEST_CONSTEXPR_CXX20 bool test_simple() {
    int a[] = {5,2,4,1,3};
    std::pop_heap(a, a+5);
    int expected[] = {4,2,3,1,5};
    assert(std::equal(a, a+5, expected, expected+5));
    return true;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93512