Re: [PATCH v3] utf8: make utf8_strwidth() and utf8_strnwidth() return size_t
- From
Hardik Kumar <hardikxk@gmail.com>
- Date
- Jul 27, 2026, 16:13 UTC
- Message-ID
- <DK9HJ1A58HMD.2CDVOK50X2UMH@gmail.com>
- In-Reply-To
- <e971400e-6d23-463f-ae9c-a21d3c5a3563@gmail.com>
On Mon Jul 27, 2026 at 6:21 PM IST, Phillip Wood wrote:
Show 26 quoted lines
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> index 48d5251..83e4dd6 100644
>> --- a/builtin/blame.c
>> +++ b/builtin/blame.c
>> @@ -564,7 +564,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent,
>> name = ci.author_mail.buf;
>> else
>> name = ci.author.buf;
>> - pad = longest_author - utf8_strwidth(name);
>> + pad = longest_author - cast_size_t_to_int(utf8_strwidth(name));
>> printf(" (%s%*s %10s",
>> name, pad, "",
>> format_time(ci.author_time,
>
> To me this example perfectly illustrates why changing the return value
> of utf8_strwidth() is a bad idea. The return value is pretty much always
> used to calculate a padding to pass to printf() which expects an int. By
> changing the return value you're forcing all the callers to do the
> conversion themselves which is a bug waiting to happen. I'm also far
> from convinced that the conversions in this patch are complete: grepping
> for 'utf8_strn\{0,1\}width' turns up several calls which do not appear
> to be correctly converted here. For example:
>
> builtin/worktree.c: display[i].width = utf8_strwidth(buf.buf);
>
> where "width" is an int.I had intentionally left out some sites which did not seem could have any impact by implicit conversions as there are other examples of such cases where the return value of `strlen` is being assigned to an int variable. Example:
in combine-diff.c (where len is an int): if (len < 0) len = strlen(line);
in builtin/update-index.c: int namelen = strlen(path);
and other such examples.
Show 6 quoted lines
> > I think it would be much better to remove the TODO comment as Junio > previously suggested and instead add some documentation to the function > explaining (a) why it is appropriate for it to return an int; (b) why we > must use the cast_size_t_to_int() helper to prevent overflows (see the > commit that added that comment).
This can result in issues down the line and I had mentioned so in a previous mail but wanted to try it with v3 since I had already been working on it. I'll send a new patch to remove the TODO. This change might just not be worth after all.
Thanks, Hardik