Re: [PATCH] object-name: declare pointer type of extend_abbrev_len()'s 2nd parameter
On 9/4/25 10:09 PM, Junio C Hamano wrote:
Show 34 quoted lines
> René Scharfe <l.s.r@web.de> writes:
>
>> Expose the expected type of the second parameter of extend_abbrev_len()
>> instead of casting a void pointer internally. Just a single caller
>> passes in a void pointer, the rest pass the correct type. Let the
>> compiler help keeping it that way.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> object-name.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> We obviously do *not* have to, but I have to wonder if we want to go
> one step further to have that single caller explicitly cast it down
> to make the intent more clear, i.e.e.g.,
>
> object-name.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git c/object-name.c w/object-name.c
> index 11aa0e6afc..8335d0239e 100644
> --- c/object-name.c
> +++ w/object-name.c
> @@ -714,7 +714,9 @@ static int repo_extend_abbrev_len(struct repository *r UNUSED,
> const struct object_id *oid,
> void *cb_data)
> {
> - return extend_abbrev_len(oid, cb_data);
> + struct min_abbrev_data *mad = cb_data;
> +
> + return extend_abbrev_len(oid, mad);
> }
>
> static void find_abbrev_len_for_midx(struct multi_pack_index *m,I can see the appeal, even though (or because) it's kinda half a step back as it keeps the original local variable, in a better place.
We could _lunge_ forward and add type checks to allow the compiler to tell us whether the pointers' journey through the void is safe. The trick below is simple enough, but requires bespoke macros AFAICS.
René
---
object-name.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
Show changes to object-name.c +26 −8
diff --git a/object-name.c b/object-name.c
index 1e0118e8a6..c37a3826d0 100644
--- a/object-name.c
+++ b/object-name.c
@@ -52,6 +52,22 @@ struct disambiguate_state {
unsigned always_call_fn:1;
};
+#define DEFINE_DISAMBIGUATE_HINT_CB(scope, fn_name) \
+scope int fn_name##__void(struct repository *r, const struct object_id *oid, \
+ void *cb_data) \
+{ \
+ return fn_name(r, oid, cb_data); \
+} \
+scope int fn_name##__void(struct repository *, const struct object_id *, void *)
+
+#define SET_DISAMBIGUATE_HINT_CB_DATA(ds, fn_name, data) do { \
+ struct disambiguate_state *dsp = (ds); \
+ if (0) \
+ fn_name(NULL, NULL, (data)); \
+ dsp->fn = fn_name##__void; \
+ dsp->cb_data = (data); \
+} while (0)
+
static void update_candidates(struct disambiguate_state *ds, const struct object_id *current)
{
/* The hash algorithm of current has already been filtered */
@@ -510,11 +526,13 @@ static int collect_ambiguous(const struct object_id *oid, void *data)
static int repo_collect_ambiguous(struct repository *r UNUSED,
const struct object_id *oid,
- void *data)
+ struct oid_array *collect)
{
- return collect_ambiguous(oid, data);
+ return collect_ambiguous(oid, collect);
}
+DEFINE_DISAMBIGUATE_HINT_CB(static, repo_collect_ambiguous);
+
static int sort_ambiguous(const void *va, const void *vb, void *ctx)
{
struct repository *sort_ambiguous_repo = ctx;
@@ -654,8 +672,7 @@ int repo_for_each_abbrev(struct repository *r, const char *prefix,
return -1;
ds.always_call_fn = 1;
- ds.fn = repo_collect_ambiguous;
- ds.cb_data = &collect;
+ SET_DISAMBIGUATE_HINT_CB_DATA(&ds, repo_collect_ambiguous, &collect);
find_short_object_filename(&ds);
find_short_packed_object(&ds);
@@ -711,11 +728,13 @@ static int extend_abbrev_len(const struct object_id *oid,
static int repo_extend_abbrev_len(struct repository *r UNUSED,
const struct object_id *oid,
- void *cb_data)
+ struct min_abbrev_data *mad)
{
- return extend_abbrev_len(oid, cb_data);
+ return extend_abbrev_len(oid, mad);
}
+DEFINE_DISAMBIGUATE_HINT_CB(static, repo_extend_abbrev_len);
+
static void find_abbrev_len_for_midx(struct multi_pack_index *m,
struct min_abbrev_data *mad)
{
@@ -871,9 +890,8 @@ int repo_find_unique_abbrev_r(struct repository *r, char *hex,
if (init_object_disambiguation(r, hex, mad.cur_len, algo, &ds) < 0)
return -1;
- ds.fn = repo_extend_abbrev_len;
ds.always_call_fn = 1;
- ds.cb_data = (void *)&mad;
+ SET_DISAMBIGUATE_HINT_CB_DATA(&ds, repo_extend_abbrev_len, &mad);
find_short_object_filename(&ds);
(void)finish_object_disambiguation(&ds, &oid_ret);
--
2.51.0
René Scharfe