Re: [PATCH v3 03/12] object-file API: add a format_object_header() function

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 17, 2022, 09:21 UTC
Message-ID: <220217.86mtipj14m.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220217045943.30223-1-worldhello.net@gmail.com>

On Thu, Feb 17 2022, Jiang Xin wrote:

Show 30 quoted lines

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> 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...

Show 7 quoted lines

> [...]
>> +	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...