On Wed, 12 Jun 2024 at 12:29, Patrick Steinhardt [off-list ref] wrote:

On Wed, Jun 12, 2024 at 11:08:14AM +0530, Chandra Pratap wrote:

quoted

In the current testing setup for infix_walk(), the following
properties of an infix traversal of a tree remain untested:
- every node of the tree must be visited
- every node must be visited exactly only

s/only/once

quoted

and only the property 'traversal in increasing order' is tested.

Nit: this reads a bit awkward. How about "In fact, we only verify that
the traversal happens in increasing order."

quoted

@@ -51,6 +50,7 @@ static void test_infix_walk(void)
 {
      struct tree_node *root = NULL;
      void *values[11] = { 0 };
+     void *out[20] = { 0 };

From the test below it looks like we only expect 11 values to be added
to `out`. Why does this array have length 20?

That's an error. I'll correct it in the next iteration.

We could of course also use something like `ALLOC_GROW()` to grow the
array dynamically. But that'd likely be overkill.

quoted

      struct curry c = { 0 };
      size_t i = 1;

@@ -59,7 +59,11 @@ static void test_infix_walk(void)
              i = (i * 7) % 11;
      } while (i != 1);

-     infix_walk(root, &check_increasing, &c);
+     c.arr = (void **) &out;

We can initialize this variable directly when declaring `c`:

    struct curry c = {
        .arr = &out;
    };

Right, this seems more concise.

Also, is the cast necessary? This is the only site where we use `struct
curry` if I'm not mistaken, so I'd expect that the type of `arr` should
match our expectations.

Trying to do this without a cast produces the following compilation error
for me:
initialization of ‘void **’ from incompatible pointer type ‘void * (*)[11]’

quoted

+     infix_walk(root, &store, &c);
+     for (i = 1; i < ARRAY_SIZE(values); i++)
+             check_pointer_eq(values + i, out[i - 1]);

Let's also verify that `c.len` matches the expected number of nodes
visited.

Sure.