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

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

René Scharfe [off-list ref] writes:

quoted

quoted

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é