On 5/24/2022 11:20 AM, Elijah Newren wrote:
> On Tue, May 24, 2022 at 7:02 AM Kevin Locke <kevin@xxxxxxxxxxxxxxx> wrote:
>>
>> On Mon, 2022-05-23 at 14:57 -0400, Derrick Stolee wrote:
>>> On 5/21/22 9:53 AM, Kevin Locke wrote:
>>>> + free((char*)tmp_original_cwd);
>>>
>>> Hm. I'm never a fan of this casting, but it existed before. It's
>>> because tmp_original_cwd is exposed globally in cache.h, which
>>> is _really widely_. However, there are only two uses: setup.c,
>>> which defines it, and common-main.c, which initializes it during
>>> process startup.
...>> This approach seems reasonable to me, as does casting to free(). It's
>> not clear to me which is preferable in this case. How to balance the
>> trade-offs between exposing const interfaces, limiting (internal)
>> interfaces to headers, and avoiding casts might be worth discussing
>> and documenting a matter of project coding style. `grep -rF 'free(('`
>> lists about 100 casts to free, suggesting the discussion may be
>> worthwhile. Introducing a free_const() macro could be another option
>> to consider.
>
> I'd prefer either a free_const() as you suggest (though as a separate
> patch from what you are submitting here), or leaving the code as-is.
> free() could have been written to take a const void* instead of just
> void*, since it's not going to modify what the pointer points at. The
> reason we call free() is because the variable isn't needed anymore,
> and using a non-const value after freeing is just as wrong as using a
> const one after freeing, so casting away the constness cannot really
> cause any new problems. So, I think the signature of free() is just
> wrong: it should have taken a const void* all along. Unfortunately,
> the wrong type signature sadly makes people feel like they have to
> choose between (a) dropping the added safety of const that the
> compiler can enforce for you during the lifetime of the variable, or
> (b) leaking memory you no longer need. I think it's a bad choice and
> you should just typecast when free'ing, but clearly others just don't
> want to see any typecasts and are willing to dispense with const on
> constant variables.
I mostly agree with you: if free() didn't have the const, then the
answer would be simple. We probably wouldn't also have the convention
of "const pointers are for memory we don't own".
Specifically with 'const char *' this can sometimes point to a
compiled string literal, so I tend to be more careful than usual
around these kinds of casts.
I'm willing to concede this point as it is much messier than just
the goals of this patch.
Thanks,
-Stolee
