On Wed, 12 Jun 2024 at 12:29, Patrick Steinhardt <ps@xxxxxx> wrote:
>
> On Wed, Jun 12, 2024 at 11:08:14AM +0530, Chandra Pratap wrote:
> > 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
>
> > 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."
>
> > @@ -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.
>
> > 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]’
> > + 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.
