Re: [PATCH v2 5/5] t-reftable-tree: improve the test for infix_walk()
- From
- Chandra Pratap <chandrapratap3519@gmail.com>
- Date
- Jun 12, 2024, 09:05 UTC
- Message-ID
- <CA+J6zkQANGzX=+=U9=DYfVujV68yE3-LvvrgF+bnYtgAskFRHg@mail.gmail.com>
- In-Reply-To
- <ZmlHZXN8_7rKLTYk@tanuki>
On Wed, 12 Jun 2024 at 12:29, Patrick Steinhardt <ps@pks.im> wrote:
Show 22 quoted lines
>
> 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.
Show 18 quoted lines
> 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]’
Show 6 quoted lines
> > + 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.