Re: [PATCH 3/3] fixup! reftable: add a heap-based priority queue for reftable records

Jeff King [off-list ref] writes:

On Wed, Sep 01, 2021 at 10:30:23PM -0700, Carlo Marcelo Arenas Belón wrote:

quoted

diff --git a/reftable/pq.c b/reftable/pq.c
index 8918d158e2..a6acca006b 100644
--- a/reftable/pq.c
+++ b/reftable/pq.c

@@ -43,12 +43,14 @@ int merged_iter_pqueue_is_empty(struct merged_iter_pqueue pq)
 
 void merged_iter_pqueue_check(struct merged_iter_pqueue pq)
 {
+#ifndef NDEBUG
 	int i = 0;
 	for (i = 1; i < pq.len; i++) {
 		int parent = (i - 1) / 2;
 
 		assert(pq_less(pq.heap[parent], pq.heap[i]));
 	}
+#endif
 }

This will trigger -Wunused-parameter warnings, since the function body
is now empty when NDEBUG is undefined. Probably switching the assert()
to die() would be better, since the whole point of the function is just
to exit on error.

If there's a problem using die() from the reftable code, it could also
return an error and the caller in the test helper could propagate it.

I agree that the patch as posted does not help but if this is
originally an assertion, then it should never trigger in real life,
so BUG() would be more appropriate than an error return, no?

On Thu, Sep 02, 2021 at 01:08:58PM -0700, Junio C Hamano wrote:

quoted

This will trigger -Wunused-parameter warnings, since the function body
is now empty when NDEBUG is undefined. Probably switching the assert()
to die() would be better, since the whole point of the function is just
to exit on error.

If there's a problem using die() from the reftable code, it could also
return an error and the caller in the test helper could propagate it.

I agree that the patch as posted does not help but if this is
originally an assertion, then it should never trigger in real life,
so BUG() would be more appropriate than an error return, no?

My thinking was that it doesn't make much sense as an assertion in the
first place. It is not a side effect of "let's make sure things are as
we expect while we're doing some other operation". The whole point of
the function is: is this data structure properly in order.

But I guess you could argue that calling the function is itself a form
of assertion. I don't really care that much either way, so whatever
Han-Wen prefers is fine with me (but I do think it is worth addressing
the warning Carlo found _somehow_).

-Peff