> The current cocci patches try to convert
> 
> 	if (!E)
> 		free(E);
> 
> into an unconditional call to free(E), with the rationale
> 
>     cocci: detect useless free(3) calls
> 
>     Add a semantic patch for removing checks that cause free(3) to only be
>     called with a NULL pointer, as that must be a programming mistake.
> 
> which came from ec6cd14c7a (cocci: detect useless free(3) calls,
> 2017-02-11).
> 
> Leaving _something_ in ALL.patch output to draw programmers'
> attention is a good thing, but this changes a piece of code that is
> originally a no-op to do something else, which may be even worse.

> We could change it to
> 
> 	if (!E)
> 		BUG("free(E) is certainly not what we meant to write");
> 
> to force programmers to think.  But it probably is safer to just
> rewrite one form of no-op into a simpler form of no-op.

> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  tools/coccinelle/free.cocci | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/tools/coccinelle/free.cocci b/tools/coccinelle/free.cocci
> index 03799e1908..3dfaae9dd8 100644
> --- a/tools/coccinelle/free.cocci
> +++ b/tools/coccinelle/free.cocci
> @@ -8,16 +8,6 @@ expression E;
>    commit_list_free(E);
>  )
>  
> -@@
> -expression E;
> -@@
> -- if (!E)
> -(
> -  free(E);
> -|
> -  commit_list_free(E);
> -)
> -
>  @@
>  expression E;
>  @@

> On 9/12/26 12:09 AM, Junio C Hamano wrote:
>> The current cocci patches try to convert
>> 
>> 	if (!E)
>> 		free(E);
>> 
>> into an unconditional call to free(E), with the rationale
>> 
>>     cocci: detect useless free(3) calls
>> 
>>     Add a semantic patch for removing checks that cause free(3) to only be
>>     called with a NULL pointer, as that must be a programming mistake.
>> 
>> which came from ec6cd14c7a (cocci: detect useless free(3) calls,
>> 2017-02-11).
>> 
>> Leaving _something_ in ALL.patch output to draw programmers'
>> attention is a good thing, but this changes a piece of code that is
>> originally a no-op to do something else, which may be even worse.
>
> Good point.  It's likely that the programmer just wanted to release the
> object in question and got the check wrong, but it's also possible that
> the free(3) call is wrong as well, and that could do real damage.
>> We could change it to
>> 
>> 	if (!E)
>> 		BUG("free(E) is certainly not what we meant to write");
>> 
>> to force programmers to think.  But it probably is safer to just
>> rewrite one form of no-op into a simpler form of no-op.
>
> With that last sentence I expected the patch to also remove the free(3)
> or commit_list_free() call, replacing the no-op with nothing, which is
> safe and simple.

> René Scharfe <l.s.r@web.de> writes:
> 
>>> We could change it to
>>>
>>> 	if (!E)
>>> 		BUG("free(E) is certainly not what we meant to write");
>>>
>>> to force programmers to think.  But it probably is safer to just
>>> rewrite one form of no-op into a simpler form of no-op.
>>
>> With that last sentence I expected the patch to also remove the free(3)
>> or commit_list_free() call, replacing the no-op with nothing, which is
>> safe and simple.
> 
> You mean
> 
> 	 if (!E)
> 	-  free(E);
> 	+  ; /* no op free(E) */
> 
> or something?  I guess we could do so, but I feared that a compiler
> that is smart enough complain and trip -Werror on us when E is too
> obviously a side-effect free expression such as a reference to a
> simple variable.