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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 14, 2021, 19:54 UTC
Message-ID: <87mtnbfk0g.fsf@evledraar.gmail.com>
In-Reply-To: <8f87cdb9-b52b-8d1a-545d-ed3055c536c0@gmail.com>

On Thu, Oct 14 2021, Phillip Wood wrote:

[Changed $subject]

Show 12 quoted lines

> 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.:

    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...

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

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