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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Hardik

On 27/07/2026 22:15, Hardik Kumar wrote:
> The `utf8_strwidth()` function is used in multiple places that all
> expect the function to return an int. The result is directly used for
> padding and width calculations and passed to `printf()` calls. All
> these operations expect the function to return an int value. Changing
> the return type here requires changing the types of all the callers and

s/requires/would require/

> other additional variables, that depend on the results from this
> function directly or indirectly, to avoid overflow by implicit
> conversions.
> 
> The comment precisely explains the reason why the explicit conversion is
> done.

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.

> - Remove an old TODO that is no longer feasible.
> - Add a comment explaining the behaviour and reason of the allowed
> expression.
> 
> Signed-off-by: Hardik Kumar <hardikxk@gmail.com>
> ---
> changes in v4:
> - drop the todo implementation and remove from codebase.
> - replace the todo with a reasonable explanation for the current
> approach and why its not worth the change.
> 
>   utf8.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> 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.

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

Phillip



>   	 */
>   	return cast_size_t_to_int(string ? width : len);
>   }