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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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};
----------------
Quuxplusone wrote:
> 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;
> }
> ```
What I want is pretty trivial, just "unroll" a few cases from your loop so they are more explicit. I want the tests to be as dumb as possible, because imagine for instance if there was a mistake in your loop conditions and it never ran? The test would be useless. Having at least a few really dumb tests removes this sort of issue.

Of course, there's a balance to strike. It doesn't always make sense to push too far with this sort of defensive behavior, but here it's pretty easy. For `make_heap`, something like this would satisfy me:

```
{
    int input[] = {3, 4, 1, 2, 5};
    std::make_heap(Iter(input), Iter(input + 5));
    assert(std::is_heap(input, input + 5));
    std::pop_heap(input, input + 5); assert(input[4] == 5);
    std::pop_heap(input, input + 4); assert(input[3] == 4);
    std::pop_heap(input, input + 3); assert(input[2] == 3);
    std::pop_heap(input, input + 2); assert(input[1] == 2);
    std::pop_heap(input, input + 1); assert(input[0] == 1);
}
```

You can put that inside `test()` itself. Does that make sense to you?

Another way to think about this: Imagine you're a user and you try to use libc++'s `std::make_heap`. Imagine the very first and most basic thing you might write just to try it out. Wouldn't it be a huge embarrassment if it didn't work for some reason? Well, we might as well just check-in a few of the most common and dumb examples of how to use facilities to guard us from that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93512