Re: [PATCH 3/4] t-reftable-tree: split test_tree() into two sub-test functions

From: Patrick Steinhardt <ps@pks.im>
Date: Jun 10, 2024, 13:49 UTC
Message-ID: <ZmcEaFxZhpyrFd-b@tanuki>
In-Reply-To: <20240610131017.8321-4-chandrapratap3519@gmail.com>

On Mon, Jun 10, 2024 at 06:31:30PM +0530, Chandra Pratap wrote:

Show 12 quoted lines

> @@ -44,13 +44,29 @@ static void test_tree(void)
>  		check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));
>  	}
>  
> -	infix_walk(root, check_increasing, &c);
> +	tree_free(root);
> +}
> +
> +static void test_infix_walk(void)
> +{
> +	struct tree_node *root = NULL;
> +	void *values[13] = { 0 };

Is there a reason why we have 13 values here while we had 11 values in the test this was split out from?

Show 7 quoted lines

> +	struct curry c = { 0 };
> +	size_t i = 1;
> +
> +	do {
> +		tree_search(values + i, &root, &test_compare, 1);
> +		i = (i * 5) % 13;
> +	} while (i != 1);

It's completely non-obvious that `tree_search()` ends up _inserting_ nodes into the tree when the entry we're searching for wasn't found (and if the last parameter is `1`. I feel like this interface could really use a complete makeover and split up its concerns. In any case, that does not need to happen as part of this patch seriesr

What I think would help though is if the commit message itself mentioned this unorthodox way of inserting values into the tree.

> +	infix_walk(root, &check_increasing, &c);

Not a fault of this commit, but this test certainly isn't great. It would succeed even if `infix_walk()` didn't do anything as we do not verify at all whether all nodes have been traversed (and traversed once, exactly).

Patrick