Re: To "const char *" and cast on free(), or "char *" and no cast...

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

Hi Ævar

On 14/10/2021 20:54, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Oct 14 2021, Phillip Wood wrote:
> 
> [Changed $subject]
> 
>> On 14/10/2021 01:10, Ævar Arnfjörð Bjarmason wrote:
>>> The "checkout" command is one of the main sources of leaks in the test
>>> suite, let's fix the common ones by not leaking from the "struct
>>> branch_info".
>>> Doing this is rather straightforward, albeit verbose, we need to
>>> xstrdup() constant strings going into the struct, and free() the ones
>>> we clobber as we go along.
>>
>> It's great to see these leaks being fixed. I wonder though if it would
>> be better to change the structure definition so that 'name' and 'path'
>> are no longer 'const'. That would be a better reflection of the new
>> regime.[...]
> 
> I think this is the right thing to do, but I'm not quite sure. There was
> a thread at it here:
> 
>      https://lore.kernel.org/git/YUZG0D5ayEWd7MLP@carlos-mbp.lan/
> 
> Where I chimed in and suggested exactly what you're saying here, but the
> consensus seemed to go the other way, and if you grep:
> 
>      git grep -F 'free((char *)'
> 
> You can see that we use this pattern pretty widely.
> 
>> It would also mean we could lose all the casts when freeing
>> and there would be a compiler warning if a string literal is assigned
>> to one of those fields.
> 
> What compiler/set of warnings gives you a warning when you do that? I
> don't get warned on e.g.:

Oh, I think I was thinking of -Wwrite-strings but we don't have that 
warning on and turning it on causes a bunch of -Wdiscarded-qualifier 
warnings.

Best Wishes

Phillip

>      diff --git a/builtin/checkout.c b/builtin/checkout.c
>      index a32af16d5e4..d7053579bdf 100644
>      --- a/builtin/checkout.c
>      +++ b/builtin/checkout.c
>      @@ -94 +94 @@ struct branch_info {
>      -       const char *name; /* The short name used */
>      +       char *name; /* The short name used */
>      @@ -110 +110 @@ static void branch_info_release(struct branch_info *info)
>      -       free((char *)info->name);
>      +       free(info->name);
>      @@ -1107 +1107 @@ static int switch_branches(const struct checkout_opts *opts,
>      -               new_branch_info->name = xstrdup("(empty)");
>      +               new_branch_info->name = "(empty)";
> 
> Now, what is really useful is making it a "char * const", especially
> when hacking up these changes as you'll find all the assignments, but I
> haven't found the general use in having that make it to a submitted
> patch, since you need to assign somewhere, and those then need to be a
> str[n]cpy() (except we banned.h it) or memcpy() with a cast...
>