threads / patch / 56426

patch, 3 partshn/reftable: fixes for building with -DNDEBUG

Subject: [PATCH 0/3] hn/reftable: fixes for building with -DNDEBUG

## tl;dr

10 messages between Sep 2, 2021 and Sep 3, 2021. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Carlo Marcelo Arenas Belón· Sep 2, 2021, 05:30 UTC · lore
they are all based against the last reroll for that topic in "seen"

they all introduce the use of a macro from git (MAYBE_UNUSED) that hopefully is not controversial (ex: when applied to libgit2) as other git-compat is already in use.

they are named so they could be easily squashed into their corresponding patches, but at least the third one needs some further evaluation, where the code in the assert gets removed and with it the whole function becomes a NOOP, even if the tests still pass.

there is also an additional fixup for supporting OpenBSD 7 (now in beta) that will be sent independently; sorry for the noise

Carlo Marcelo Arenas Belón (3):
  fixup! refs: RFC: Reftable support for git-core
  fixup! reftable: add merged table view
  fixup! reftable: add a heap-based priority queue for reftable records
 refs/reftable-backend.c | 5 +++--
 reftable/merged_test.c  | 8 ++++----
 reftable/pq.c           | 2 ++
 3 files changed, 9 insertions(+), 6 deletions(-)
-- 
2.33.0.481.g26d3bed244
Carlo Marcelo Arenas Belón· Sep 2, 2021, 05:30 UTC · re: Carlo Marcelo Arenas Belón · lore

[PATCH 1/3] fixup! refs: RFC: Reftable support for git-core

need to reorder the variables to hopefully make it easier to see why they might not be used since assert will compile out itself with -DNDEBUG.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 refs/reftable-backend.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to refs/reftable-backend.c +3 −2
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 61ee144e19..5d733b0496 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -127,10 +127,11 @@ static void clear_reftable_log_record(struct reftable_log_record *log)
 
 static void fill_reftable_log_record(struct reftable_log_record *log)
 {
-	const char *info = git_committer_info(0);
 	struct ident_split split = { NULL };
-	int result = split_ident_line(&split, info, strlen(info));
 	int sign = 1;
+	MAYBE_UNUSED const char *info = git_committer_info(0);
+	MAYBE_UNUSED int result = split_ident_line(&split, info, strlen(info));
+
 	assert(0 == result);
 
 	reftable_log_record_release(log);

-- 
2.33.0.481.g26d3bed244
Jeff King· Sep 2, 2021, 09:05 UTC · re: Carlo Marcelo Arenas Belón · lore

Re: [PATCH 1/3] fixup! refs: RFC: Reftable support for git-core

On Wed, Sep 01, 2021 at 10:30:21PM -0700, Carlo Marcelo Arenas Belón wrote:
> need to reorder the variables to hopefully make it easier to see why
> they might not be used since assert will compile out itself with -DNDEBUG.

This should probably lead with the reason for the patch (avoiding errors with NDEBUG), and then mention any other bits (like "we also reorder for clarity"). That makes the point of the patch easier to see.

Show 14 quoted lines
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 61ee144e19..5d733b0496 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -127,10 +127,11 @@ static void clear_reftable_log_record(struct reftable_log_record *log)
>  
>  static void fill_reftable_log_record(struct reftable_log_record *log)
>  {
> -	const char *info = git_committer_info(0);
>  	struct ident_split split = { NULL };
> -	int result = split_ident_line(&split, info, strlen(info));
>  	int sign = 1;
> +	MAYBE_UNUSED const char *info = git_committer_info(0);
> +	MAYBE_UNUSED int result = split_ident_line(&split, info, strlen(info));

I don't think you need MAYBE_UNUSED on "info"; it is always used, even if the assert isn't compiled.

>  	assert(0 == result);
IMHO this would be better converted to:
  if (result)
	BUG("unable to parse output of get_committer_info()");

That solves the problem, avoids introducing head-scratching constructs like MAYBE_UNUSED, and gives readers more of a clue about what we expected.

-Peff
PS I do think it's pretty ugly that we have to re-split the ident from
   git_committer_info(), which just took the individual pieces and
   stuffed them into a string! But our ident functions are a little
   cumbersome here (we even have fmt_name(), but I think it's too picky
   about IDENT_STRICT). I suspect it could be fixed with some
   refactoring, but that's way out of scope for your series (and for the
   reftable series itself), so let's leave it for now.
Han-Wen Nienhuys· Sep 2, 2021, 09:26 UTC · re: Jeff King · lore

Re: [PATCH 1/3] fixup! refs: RFC: Reftable support for git-core

On Thu, Sep 2, 2021 at 11:05 AM Jeff King <peff@peff.net> wrote:
Show 9 quoted lines
>
> On Wed, Sep 01, 2021 at 10:30:21PM -0700, Carlo Marcelo Arenas Belón wrote:
>
> > need to reorder the variables to hopefully make it easier to see why
> > they might not be used since assert will compile out itself with -DNDEBUG.
>
> This should probably lead with the reason for the patch (avoiding errors
> with NDEBUG), and then mention any other bits (like "we also reorder for
> clarity"). That makes the point of the patch easier to see.
Counterpoint: I can see what the problem is (thanks, Carlo!) but would
likely fix it in a different way, so effort spent in polishing commit
messages will be wasted.
-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Carlo Marcelo Arenas Belón· Sep 2, 2021, 05:30 UTC · re: Carlo Marcelo Arenas Belón · lore

[PATCH 2/3] fixup! reftable: add merged table view

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
Just like in the next patch, having relevant code in the assert might be
a problem, but this is mostly test code and still passes.
 reftable/merged_test.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to reftable/merged_test.c +4 −4
diff --git a/reftable/merged_test.c b/reftable/merged_test.c
index f58e44003e..b7be4dcf9f 100644
--- a/reftable/merged_test.c
+++ b/reftable/merged_test.c
@@ -47,8 +47,8 @@ static void write_test_table(struct strbuf *buf,
 	reftable_writer_set_limits(w, min, max);
 
 	for (i = 0; i < n; i++) {
-		uint64_t before = refs[i].update_index;
-		int n = reftable_writer_add_ref(w, &refs[i]);
+		MAYBE_UNUSED uint64_t before = refs[i].update_index;
+		MAYBE_UNUSED int n = reftable_writer_add_ref(w, &refs[i]);
 		assert(n == 0);
 		assert(before == refs[i].update_index);
 	}
@@ -181,7 +181,7 @@ static void test_merged(const char *fn_name)
 		},
 	};
 
-	struct reftable_ref_record want[] = {
+	MAYBE_UNUSED struct reftable_ref_record want[] = {
 		r2[0],
 		r1[1],
 		r3[0],
@@ -251,7 +251,7 @@ static void test_default_write_opts(const char *fn_name)
 	int err;
 	struct reftable_block_source source = { NULL };
 	struct reftable_table *tab = reftable_calloc(sizeof(*tab) * 1);
-	uint32_t hash_id;
+	MAYBE_UNUSED uint32_t hash_id;
 	struct reftable_reader *rd = NULL;
 	struct reftable_merged_table *merged = NULL;
 

-- 
2.33.0.481.g26d3bed244
Carlo Marcelo Arenas Belón· Sep 2, 2021, 05:30 UTC · re: Carlo Marcelo Arenas Belón · lore

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

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(+)
Show changes to reftable/pq.c +2 −0
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)

-- 
2.33.0.481.g26d3bed244
Jeff King· Sep 2, 2021, 09:09 UTC · re: Carlo Marcelo Arenas Belón · lore

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

On Wed, Sep 01, 2021 at 10:30:23PM -0700, Carlo Marcelo Arenas Belón wrote:
Show 17 quoted lines
> 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
Junio C Hamano· Sep 2, 2021, 20:08 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 27 quoted lines
> 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?

Jeff King· Sep 2, 2021, 22:40 UTC · re: Junio C Hamano · lore

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

On Thu, Sep 02, 2021 at 01:08:58PM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> > 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
Junio C Hamano· Sep 3, 2021, 04:42 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
>> 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.

Show 6 quoted lines
> 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

← back to recent threads