[tarantool-patches] Re: [PATCH v5 3/4] salad: introduce bps_tree_delete_identical routine

[Kirill Shcherbatov]

>>  #ifndef BPS_TREE_IDENTICAL
>>  #define BPS_TREE_IDENTICAL(a, b) (a == b)
>> +#define BPS_TREE_IDENTICAL_STD 1
> 
> This looks ugly. Let's require the caller to always define
> BPS_TREE_IDENTICAL - it's not a big deal.

I have considered this option, maybe it is better.
Done.

>> +/**
>> + * @brief Delete an identical element from a tree (unlike
>> + * bps_tree_delete this routine relies on BPS_TREE_IDENTICAL
>> + * instead of BPS_TREE_COMPARE).
>> + * @param tree - pointer to a tree
>> + * @param elem - the element tot delete
>> + * @return - true on success or false if the element was not found in tree
> 
> I guess it should also return -1 if the element was found but not
> deleted, in other words it should return 0 iff the element has been
> deleted.

	Done:

	struct bps_leaf *leaf = leaf_path_elem.block;
	if (!BPS_TREE_IDENTICAL(elem,
				leaf->elems[leaf_path_elem.insertion_point]))
		return -1;
	bps_tree_process_delete_leaf(tree, &leaf_path_elem);

> 
> Could you please reuse the existing tree for the test rather than
> introducing a new one?

Unfortunately, it is not possible. All other tests don't use structure as a leaf.
> Please check the return value as well.

	if (struct_tree_delete_identical(&tree, e2) == 0)
		fail("deletion of the non-identical element must fail", "false");
	if (struct_tree_find(&tree, 1) == NULL)
		fail("test non-identical element deletion failure", "false");
	if (struct_tree_delete_identical(&tree, e1) != 0)
		fail("deletion of the identical element must not fail", "false");
	if (struct_tree_find(&tree, 1) != NULL)
		fail("test identical element deletion completion", "false");

============================================

Updated on branch.