On Thu, Feb 17 2022, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx>
>
> On Sat, Feb 5, 2022 Ævar Arnfjörð Bjarmason wrote:
>
>> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
>> index 2b2e28bad79..123df7d9a53 100644
>> --- a/builtin/fast-import.c
>> +++ b/builtin/fast-import.c
>> @@ -937,8 +937,8 @@ static int store_object(
>> git_hash_ctx c;
>> git_zstream s;
>>
>> - hdrlen = xsnprintf((char *)hdr, sizeof(hdr), "%s %lu",
>> - type_name(type), (unsigned long)dat->len) + 1;
>> + hdrlen = format_object_header((char *)hdr, sizeof(hdr), type,
>> + dat->len);
>
> Type casting can be avoid if we use "void *" as the first parameter of
> "format_object_header", and do type casting inside the helper function.
> [...]
> The return value of `type_name(type)` has not been checked for the original
> implement, how about write a online inline-function in a header file like this:
>
> static inline int format_object_header(void *str, size_t size,
> const char *type_name,
> size_t objsize)
> {
> return xsnprintf((char *)str, size, "%s %"PRIuMAX, type_name,
> (uintmax_t)objsize) + 1;
> }
I don't think the casting in the callers is bad, in that for the callers
that do use "char *" we get the compiler to help us with type checking.
Using a void * is something we really reserve only for callback-type
values, because it mens that now nobody gets any type checking.
I think if we wanted to avoid the casts it would make more sense to add
a trivial ucformat_object_header() wrapper or whatever, which would take
"unsigned chan *" and do the cast, or just tweak the relevant calling
code to change the type (IIRC some of it used unsigned v.s. signed for
no particular reason).
But I think just leaving this part as it is is better here...
> [...]
>> + if (!name)
>> + BUG("could not get a type name for 'enum object_type' value %d", type);
>> +
>
> The return value of `type_name(type)` has not been checked for the original
> implement, how about write a online inline-function in a header file like this:
Yes, this part is not a faithful conversion on my part, but I think it
made sense when converting this to a library function.
The alternative is that we'd segfault on some platforms (not glibc,
since it's OK with null %s arguments), just checking it is cheap & I
think a good sanity check...
