| Message ID | 20210902053023.44006-4-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hn/reftable: fixes for building with -DNDEBUG | expand |
On Wed, Sep 01, 2021 at 10:30:23PM -0700, Carlo Marcelo Arenas Belón wrote: > 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. -Peff
Jeff King <peff@peff.net> writes: > On Wed, Sep 01, 2021 at 10:30:23PM -0700, Carlo Marcelo Arenas Belón wrote: > >> 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: > > 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
Jeff King <peff@peff.net> writes: >> 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. Very true. Ah, so you mean the way this function is supposed to be used is to _call_ it, like so: if (!is_our_data_structure_healthy()) BUG(...); It makes it easier to reason about what the function is doing, I guess. > 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
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 } struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq)
not a very useful option, but the fact the main logic is done inside and assert that then gets compiled out when -DNDEBUG might be worth reconsidering. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- reftable/pq.c | 2 ++ 1 file changed, 2 insertions(+)