Re: [PATCH v4] utf8: replace utf8_strwidth todo with descriptive comment

Phillip Wood <[email protected]> writes:

> I don't think this comment, or the lines below add anything useful to 
> the message. It would be better to say something like
>
> As we do not want to change the return type, update the comment to 
> explain that and the need for the explicit cast.

Perfect ;-)

>> diff --git a/utf8.c b/utf8.c
>> index 96460cc..1b55bd4 100644
>> --- a/utf8.c
>> +++ b/utf8.c
>> @@ -227,8 +227,9 @@ int utf8_strnwidth(const char *string, size_t len, int skip_ansi)
>>   	}
>>   
>>   	/*
>> -	 * TODO: fix the interface of this function and `utf8_strwidth()` to
>> -	 * return `size_t` instead of `int`.
>> +	 * The function is used in multiple locations where the callers
>> +	 * expect the result to be a signed int value. We cast the
>> +	 * result to an int to avoid changing signatures of all callers.
>
> The last sentence does not really capture the reasons given in the 
> message of the commit that added this comment. If you haven't done so 
> already you should read it - see 937b71cc8b (utf8: fix overflow when 
> returning string width, 2022-12-01). The fundamental reason to call 
> cast_size_t_to_int(), rather than relying on an implicit conversion to 
> the return type, is not about changing signatures, it is about avoiding 
> an overflow that caused git to crash.

The comment should also answer why the callers want an int, and
whether that is a legitimate need.  Topics the comment may want to
cover include:

   - Callers want display width; we will never deal with output
     wider than 2 billion columns, so int is adequate, provided we
     do not cause bugs due to integer wraparound.

   - The return value is used to compute width in constructs like:

        printf("%*s", width, string)

     which requires int, not size_t.  Instead of forcing these
     callers to call cast_size_t_to_int() individually, this
     function should return int after ensuring the value is correct
     without wraparound.

This is in addition to explaining why we want cast_size_t_to_int(),
as you described above.

> When you send a new version of the patch please CC everyone who 
> commented on previous versions so they don't have to trawl the list to 
> find it.

Thanks.