Re: [PATCH 03/12] ls-files: free max_prefix when done

From: Andrzej Hunt <andrzej@ahunt.org>
Date: Apr 25, 2021, 13:16 UTC
Message-ID: <11ae3ce9-997b-32fc-7bc3-ee95a3d99153@ahunt.org>
In-Reply-To: <6a72a920-134f-541b-7caa-debe24658005@web.de>

On 10/04/2021 10:12, René Scharfe wrote:

Show 21 quoted lines

> Am 09.04.21 um 20:47 schrieb Andrzej Hunt via GitGitGadget:
>> From: Andrzej Hunt <ajrhunt@google.com>
>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
>> index 60a2913a01e9..53e20bbf9cce 100644
>> --- a/builtin/ls-files.c
>> +++ b/builtin/ls-files.c
>> @@ -781,5 +781,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
>>   	}
>>
>>   	dir_clear(&dir);
>> +	free((void *)max_prefix);
> 
> This cast is necessary to ignore the const attribute of the pointer.
> It's scary, but safe here because this function owns the referenced
> object.
> 
> I think the promise to not modify the string given at the top of the
> function is not worth having to take back that promise forcefully at
> the end to dispose of it.  Determining the correctness of this cast
> requires reading the whole function.  Removing the const from the
> declaration (and the cast) would improve readability overall.  Thoughts?

I agree - I'll change this in V2 V2. In fact, Peff already given the following explanation for why non-const is preferred in this scenario on a previous patch of mine (which I failed to heed when preparing this patch):

 > If a variable is meant to take ownership of memory, our usual
 > convention is to not declare it as "const"."
https://lore.kernel.org/git/YEZ0jLppB9wOg%2Faf@coredump.intra.peff.net/

Show 5 quoted lines

> 
>>   	return 0;
>>   }
>>
>