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

From: René Scharfe <l.s.r@web.de>
Date: Sep 13, 2026, 10:45 UTC
Message-ID: <96aca004-0df4-4e21-b60a-0288239122cc@web.de>
In-Reply-To: <xmqqcxui8f0c.fsf@gitster.g>

On 9/12/26 9:07 PM, Junio C Hamano wrote:

Show 24 quoted lines

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

GCC apparently accepts "if (!E);", Clang warns. Both currently accept "if (!E) {}". See https://godbolt.org/z/9h8YdjrGP for some more variants. Other compilers or versions could react differently, of course.

I would have just removed everything:

   -  if (!E) free(E);

, risking the loss of side-effects and welcoming any warnings, accepting that this bluntness would be rude and potentially unsafe. That's a bit like in the "computer says no" skits, I realize now.

I agree that the polite thing to do is to leave the flawed code in and let the programmer find out that it's not doing anything some other way. No need to put up a targeted defense against this inconsequential and unlikely mistake.

René