On 24/07/18 01:10AM, Karthik Nayak wrote:

Chandra Pratap [off-list ref] writes:

quoted

On Thu, 18 Jul 2024 at 03:45, Justin Tobler [off-list ref] wrote:

quoted

On 24/07/17 08:00PM, Chandra Pratap wrote:

quoted

quoted

The `i = (i * 7) % 11;` is used to deterministically generate numbers
1-10 in a psuedo-random fashion. These numbers are used as memory
offsets to be inserted into the tree. I suspect the psuedo-randomness is
useful keys should be ordered when inserted into the tree and that is
later validated as part of the in-order traversal that is performed.

That's right, the randomness of the insertion order is helpful in validating
that the tree-functions 'tree_search()' and 'infix_walk()' work according
to their defined behaviour.

quoted

While rather compact, I find the test setup here to rather difficult to
parse. It might be a good idea to either provide comments explaining
this test setup or consider refactoring it. Honestly, I'd personally
perfer the tree setup be done more explicitly as I think it would make
understanding the test much easier.

This probably ties in with the comments by Patrick on the previous iteration
of this patch, that using 'tree_search()' to insert tree nodes leads to
confusion. Solving that would require efforts outside the scope of this
patch series though, so I'm more inclined towards providing comments
and other ways of simplifying this subroutine.

Agreed that refactoring `tree_search()` probably is out of scope here.
But rewriting the test is definitely something we can do.

Perhaps:

static void t_tree(void)
{
	struct tree_node *root = NULL;
	int values[11] = {7, 5, 2, 3, 10, 4, 6, 9, 8, 1};
	struct tree_node *nodes[11] = { 0 };
	size_t i = 1;
	struct curry c = { 0 };

    // Insert values to the tree by passing '1' as the last argument.
    for (i = 1; i < ARRAY_SIZE(values); i++) {
		nodes[i] = tree_search(&values[i], &root, &t_compare, 1);
    }
	
	for (i = 1; i < ARRAY_SIZE(nodes); i++) {
		check_pointer_eq(values[i], nodes[i]->key);
		check_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));
	}

	infix_walk(root, check_increasing, &c);
	tree_free(root);
}

Wouldn't this have the same effect while making it much easier to read?

Personally, I quite like this approach. It's more up front with what its
doing and ultimately accoplishes the same thing.