diff mbox series

[v4,14/18] reftable/basics: stop using `UNUSED` annotation

Message ID 20250206-pks-reftable-drop-git-compat-util-v4-14-603d276d5f95@pks.im (mailing list archive)
State Superseded
Headers show
Series reftable: stop using "git-compat-util.h" | expand

Commit Message

Patrick Steinhardt Feb. 6, 2025, 7:52 a.m. UTC 
Stop using the `UNUSED` annotation and replace it with a new
`REFTABLE_UNUSED` macro. The latter is a weaker guarantee compared to
`UNUSED` as it only suppresses unused parameters without generating a
warning in case a parameter marked as unused is in fact used. But it's
good enough, and by relaxing the behaviour a bit we avoid having to wire
up compiler-specific logic.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/basics.h      |  2 ++
 reftable/blocksource.c | 10 +++++++---
 reftable/iter.c        | 17 ++++++++++++-----
 reftable/record.c      | 51 ++++++++++++++++++++++++++++++++++++--------------
 reftable/writer.c      |  4 +++-
 5 files changed, 61 insertions(+), 23 deletions(-)

Comments

Toon Claes Feb. 7, 2025, 10:03 a.m. UTC | #1 
Patrick Steinhardt <ps@pks.im> writes:

> Stop using the `UNUSED` annotation and replace it with a new
> `REFTABLE_UNUSED` macro. The latter is a weaker guarantee compared to
> `UNUSED` as it only suppresses unused parameters without generating a
> warning in case a parameter marked as unused is in fact used. But it's
> good enough, and by relaxing the behaviour a bit we avoid having to wire
> up compiler-specific logic.

I see MAYBE_UNUSED is defined as `__attribute__((__unused__))`, which as
far as I can tell is independent on the compiler, and has the same
effect as the implementation in this patch. Would it make sense to use
that defition instead? Or did you intentionally choose a statement that
will sit on a separate line so it's more obviously that line needs to be
deleted whenever the parameter is put in use?
Patrick Steinhardt Feb. 7, 2025, 11:50 a.m. UTC | #2 
On Fri, Feb 07, 2025 at 11:03:15AM +0100, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Stop using the `UNUSED` annotation and replace it with a new
> > `REFTABLE_UNUSED` macro. The latter is a weaker guarantee compared to
> > `UNUSED` as it only suppresses unused parameters without generating a
> > warning in case a parameter marked as unused is in fact used. But it's
> > good enough, and by relaxing the behaviour a bit we avoid having to wire
> > up compiler-specific logic.
> 
> I see MAYBE_UNUSED is defined as `__attribute__((__unused__))`, which as
> far as I can tell is independent on the compiler, and has the same
> effect as the implementation in this patch. Would it make sense to use
> that defition instead? Or did you intentionally choose a statement that
> will sit on a separate line so it's more obviously that line needs to be
> deleted whenever the parameter is put in use?

Interesting. I was checking the definition of `UNUSED`, which depends on
the actual compiler we're using. But you're right, `MAYBE_UNUSED` is
defined unconditionally, so by that reasoning we should be able to use
it on all supported platforms indeed.

I'll revise this patch, thanks for noticing!

Patrick
diff mbox series

Patch

diff --git a/reftable/basics.h b/reftable/basics.h
index 59000798f0..4d0645a4e9 100644
--- a/reftable/basics.h
+++ b/reftable/basics.h
@@ -16,6 +16,8 @@  license that can be found in the LICENSE file or at
 #include "system.h"
 #include "reftable-basics.h"
 
+#define REFTABLE_UNUSED(x) (void)(x)
+
 struct reftable_buf {
 	size_t alloc;
 	size_t len;
diff --git a/reftable/blocksource.c b/reftable/blocksource.c
index 02972c46f4..bfd64b0e48 100644
--- a/reftable/blocksource.c
+++ b/reftable/blocksource.c
@@ -13,15 +13,17 @@  license that can be found in the LICENSE file or at
 #include "reftable-blocksource.h"
 #include "reftable-error.h"
 
-static void reftable_buf_return_block(void *b UNUSED, struct reftable_block *dest)
+static void reftable_buf_return_block(void *b, struct reftable_block *dest)
 {
+	REFTABLE_UNUSED(b);
 	if (dest->len)
 		memset(dest->data, 0xff, dest->len);
 	reftable_free(dest->data);
 }
 
-static void reftable_buf_close(void *b UNUSED)
+static void reftable_buf_close(void *b)
 {
+	REFTABLE_UNUSED(b);
 }
 
 static ssize_t reftable_buf_read_block(void *v, struct reftable_block *dest,
@@ -67,8 +69,10 @@  static uint64_t file_size(void *b)
 	return ((struct file_block_source *)b)->size;
 }
 
-static void file_return_block(void *b UNUSED, struct reftable_block *dest UNUSED)
+static void file_return_block(void *b, struct reftable_block *dest)
 {
+	REFTABLE_UNUSED(b);
+	REFTABLE_UNUSED(dest);
 }
 
 static void file_close(void *v)
diff --git a/reftable/iter.c b/reftable/iter.c
index b2ffb09c16..452add2705 100644
--- a/reftable/iter.c
+++ b/reftable/iter.c
@@ -25,18 +25,23 @@  int iterator_next(struct reftable_iterator *it, struct reftable_record *rec)
 	return it->ops->next(it->iter_arg, rec);
 }
 
-static int empty_iterator_seek(void *arg UNUSED, struct reftable_record *want UNUSED)
+static int empty_iterator_seek(void *arg, struct reftable_record *want)
 {
+	REFTABLE_UNUSED(arg);
+	REFTABLE_UNUSED(want);
 	return 0;
 }
 
-static int empty_iterator_next(void *arg UNUSED, struct reftable_record *rec UNUSED)
+static int empty_iterator_next(void *arg, struct reftable_record *rec)
 {
+	REFTABLE_UNUSED(arg);
+	REFTABLE_UNUSED(rec);
 	return 1;
 }
 
-static void empty_iterator_close(void *arg UNUSED)
+static void empty_iterator_close(void *arg)
 {
+	REFTABLE_UNUSED(arg);
 }
 
 static struct reftable_iterator_vtable empty_vtable = {
@@ -143,9 +148,11 @@  static int indexed_table_ref_iter_next_block(struct indexed_table_ref_iter *it)
 	return 0;
 }
 
-static int indexed_table_ref_iter_seek(void *p UNUSED,
-				       struct reftable_record *want UNUSED)
+static int indexed_table_ref_iter_seek(void *p,
+				       struct reftable_record *want)
 {
+	REFTABLE_UNUSED(p);
+	REFTABLE_UNUSED(want);
 	return REFTABLE_API_ERROR;
 }
 
diff --git a/reftable/record.c b/reftable/record.c
index 9a1edf39a0..5ee2fe44a7 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -490,11 +490,13 @@  static void reftable_obj_record_release(void *rec)
 }
 
 static int reftable_obj_record_copy_from(void *rec, const void *src_rec,
-					 uint32_t hash_size UNUSED)
+					 uint32_t hash_size)
 {
 	struct reftable_obj_record *obj = rec;
 	const struct reftable_obj_record *src = src_rec;
 
+	REFTABLE_UNUSED(hash_size);
+
 	reftable_obj_record_release(obj);
 
 	REFTABLE_ALLOC_ARRAY(obj->hash_prefix, src->hash_prefix_len);
@@ -528,13 +530,16 @@  static uint8_t reftable_obj_record_val_type(const void *rec)
 }
 
 static int reftable_obj_record_encode(const void *rec, struct string_view s,
-				      uint32_t hash_size UNUSED)
+				      uint32_t hash_size)
 {
 	const struct reftable_obj_record *r = rec;
 	struct string_view start = s;
 	int i = 0;
 	int n = 0;
 	uint64_t last = 0;
+
+	REFTABLE_UNUSED(hash_size);
+
 	if (r->offset_len == 0 || r->offset_len >= 8) {
 		n = put_var_int(&s, r->offset_len);
 		if (n < 0) {
@@ -563,8 +568,8 @@  static int reftable_obj_record_encode(const void *rec, struct string_view s,
 
 static int reftable_obj_record_decode(void *rec, struct reftable_buf key,
 				      uint8_t val_type, struct string_view in,
-				      uint32_t hash_size UNUSED,
-				      struct reftable_buf *scratch UNUSED)
+				      uint32_t hash_size,
+				      struct reftable_buf *scratch)
 {
 	struct string_view start = in;
 	struct reftable_obj_record *r = rec;
@@ -572,6 +577,9 @@  static int reftable_obj_record_decode(void *rec, struct reftable_buf key,
 	int n = 0;
 	uint64_t last;
 
+	REFTABLE_UNUSED(hash_size);
+	REFTABLE_UNUSED(scratch);
+
 	reftable_obj_record_release(r);
 
 	REFTABLE_ALLOC_ARRAY(r->hash_prefix, key.len);
@@ -618,17 +626,20 @@  static int reftable_obj_record_decode(void *rec, struct reftable_buf key,
 	return start.len - in.len;
 }
 
-static int not_a_deletion(const void *p UNUSED)
+static int not_a_deletion(const void *p)
 {
+	REFTABLE_UNUSED(p);
 	return 0;
 }
 
 static int reftable_obj_record_equal_void(const void *a, const void *b,
-					  uint32_t hash_size UNUSED)
+					  uint32_t hash_size)
 {
 	struct reftable_obj_record *ra = (struct reftable_obj_record *) a;
 	struct reftable_obj_record *rb = (struct reftable_obj_record *) b;
 
+	REFTABLE_UNUSED(hash_size);
+
 	if (ra->hash_prefix_len != rb->hash_prefix_len
 	    || ra->offset_len != rb->offset_len)
 		return 0;
@@ -1054,12 +1065,14 @@  static int reftable_index_record_key(const void *r, struct reftable_buf *dest)
 }
 
 static int reftable_index_record_copy_from(void *rec, const void *src_rec,
-					   uint32_t hash_size UNUSED)
+					   uint32_t hash_size)
 {
 	struct reftable_index_record *dst = rec;
 	const struct reftable_index_record *src = src_rec;
 	int err;
 
+	REFTABLE_UNUSED(hash_size);
+
 	reftable_buf_reset(&dst->last_key);
 	err = reftable_buf_add(&dst->last_key, src->last_key.buf, src->last_key.len);
 	if (err < 0)
@@ -1075,19 +1088,23 @@  static void reftable_index_record_release(void *rec)
 	reftable_buf_release(&idx->last_key);
 }
 
-static uint8_t reftable_index_record_val_type(const void *rec UNUSED)
+static uint8_t reftable_index_record_val_type(const void *rec)
 {
+	REFTABLE_UNUSED(rec);
 	return 0;
 }
 
 static int reftable_index_record_encode(const void *rec, struct string_view out,
-					uint32_t hash_size UNUSED)
+					uint32_t hash_size)
 {
 	const struct reftable_index_record *r =
 		(const struct reftable_index_record *)rec;
 	struct string_view start = out;
+	int n;
 
-	int n = put_var_int(&out, r->offset);
+	REFTABLE_UNUSED(hash_size);
+
+	n = put_var_int(&out, r->offset);
 	if (n < 0)
 		return n;
 
@@ -1097,15 +1114,19 @@  static int reftable_index_record_encode(const void *rec, struct string_view out,
 }
 
 static int reftable_index_record_decode(void *rec, struct reftable_buf key,
-					uint8_t val_type UNUSED,
+					uint8_t val_type,
 					struct string_view in,
-					uint32_t hash_size UNUSED,
-					struct reftable_buf *scratch UNUSED)
+					uint32_t hash_size,
+					struct reftable_buf *scratch)
 {
 	struct string_view start = in;
 	struct reftable_index_record *r = rec;
 	int err, n = 0;
 
+	REFTABLE_UNUSED(val_type);
+	REFTABLE_UNUSED(hash_size);
+	REFTABLE_UNUSED(scratch);
+
 	reftable_buf_reset(&r->last_key);
 	err = reftable_buf_add(&r->last_key, key.buf, key.len);
 	if (err < 0)
@@ -1120,11 +1141,13 @@  static int reftable_index_record_decode(void *rec, struct reftable_buf key,
 }
 
 static int reftable_index_record_equal(const void *a, const void *b,
-				       uint32_t hash_size UNUSED)
+				       uint32_t hash_size)
 {
 	struct reftable_index_record *ia = (struct reftable_index_record *) a;
 	struct reftable_index_record *ib = (struct reftable_index_record *) b;
 
+	REFTABLE_UNUSED(hash_size);
+
 	return ia->offset == ib->offset && !reftable_buf_cmp(&ia->last_key, &ib->last_key);
 }
 
diff --git a/reftable/writer.c b/reftable/writer.c
index 5961698311..0040a1b1c4 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -636,10 +636,12 @@  static void write_object_record(void *void_arg, void *key)
 done:;
 }
 
-static void object_record_free(void *void_arg UNUSED, void *key)
+static void object_record_free(void *void_arg, void *key)
 {
 	struct obj_index_tree_node *entry = key;
 
+	REFTABLE_UNUSED(void_arg);
+
 	REFTABLE_FREE_AND_NULL(entry->offsets);
 	reftable_buf_release(&entry->hash);
 	reftable_free(entry);