v5-0003-review.patch

From f2b54fbb2bc0b6a74d10f46b086e238d76fe822f Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
Date: Fri, 23 Feb 2024 13:32:04 +0100
Subject: [PATCH v5 3/5] review

---
 src/common/binaryheap.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/src/common/binaryheap.c b/src/common/binaryheap.c
index ff03c477dc9..f656c47524e 100644
--- a/src/common/binaryheap.c
+++ b/src/common/binaryheap.c
@@ -54,6 +54,8 @@ static void sift_up(binaryheap *heap, int node_off);
  * store the given number of nodes, with the heap property defined by
  * the given comparator function, which will be invoked with the additional
  * argument specified by 'arg'.
+ *
+ * XXX Should document the new "indexed" argument.
  */
 binaryheap *
 binaryheap_allocate(int capacity, binaryheap_comparator compare,
@@ -110,6 +112,7 @@ binaryheap_free(binaryheap *heap)
 {
 	if (heap->bh_indexed)
 		bh_nodeidx_destroy(heap->bh_nodeidx);
+
 	pfree(heap);
 }
 
@@ -152,28 +155,34 @@ bh_enlarge_node_array(binaryheap *heap)
 }
 
 /*
- * Set the given node at the 'idx' and updates its position accordingly.
+ * Set the given node at the 'index' and updates its position accordingly.
+ *
+ * XXX No need to shorten the argument names, I think.
+ *
+ * XXX Should this return "found" maybe?
  */
 static void
-bh_set_node(binaryheap *heap, bh_node_type d, int idx)
+bh_set_node(binaryheap *heap, bh_node_type node, int index)
 {
 	bh_nodeidx_entry *ent;
 	bool	found;
 
 	/* Set the node to the nodes array */
-	heap->bh_nodes[idx] = d;
+	heap->bh_nodes[index] = node;
 
 	if (heap->bh_indexed)
 	{
 		/* Remember its index in the nodes array */
-		ent = bh_nodeidx_insert(heap->bh_nodeidx, d, &found);
-		ent->idx = idx;
+		ent = bh_nodeidx_insert(heap->bh_nodeidx, node, &found);
+		ent->idx = index;
 	}
 }
 
 /*
  * Replace the node at 'idx' with the given node 'replaced_by'. Also
  * update their positions accordingly.
+ *
+ * XXX can we do Assert(found) here? if bh_set_node returns it, ofc
  */
 static void
 bh_replace_node(binaryheap *heap, int idx, bh_node_type replaced_by)
@@ -280,6 +289,8 @@ binaryheap_remove_first(binaryheap *heap)
 	{
 		heap->bh_size--;
 
+		/* XXX maybe it'd be good to make the check in bh_nodeidx_delete, so that
+		 * we don't need to do it everywhere. */
 		if (heap->bh_indexed)
 			bh_nodeidx_delete(heap->bh_nodeidx, result);
 
-- 
2.43.0