Re: [PATCH] cocci: remove risky "if (!E) free(E)" conversion

[Junio C Hamano <gitster@pobox.com>]

René Scharfe <l.s.r@web.de> writes:

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

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.