[Gluster-devel] useless-if-before-free [Re: Change in glusterfs[release-3.2]: nfs: memory leak fixes

[Jim Meyering]

Kaleb KEITHLEY (Code Review) wrote:
> Kaleb KEITHLEY has posted comments on this change.
>
> Change subject: nfs: memory leak fixes
> ......................................................................
>
>
> Patch Set 1: (3 inline comments)
>
> ....................................................
> File xlators/nfs/server/src/nfs-common.c
> Line 325:         GF_FREE (path);
> I notice that in other calls to
> inode_path()/nfs_loc_fill()/GF_FREE(path) that path (or resolvedpath)
> is checked for non-null before the GF_FREE(), even though it looks
> like inode_path() must have allocated memory if it didn't return an
> error.
...
> To view, visit http://review.gluster.com/3641

Hi guys,
Any if-before-GF_FREE is almost certainly wasted effort,
since GF_FREE already tests for NULL pointers.
The added conditional generally makes the code
a little harder to read/maintain.  (i.e., above)

What do you think about removing all of them?
If there's interest, I'd be happy to do it.
Running my little useless-if-before-free (find and/or remove)
  http://git.sv.gnu.org/cgit/gnulib.git/tree/build-aux/useless-if-before-free
script, I see that there are nearly 200 of those when considering
solely if-before-"free".  Considering GF_FREE, I see an additional
700+ useless tests.

Some argue that it's best to test before free to avoid the overhead
of a function call, but so far I've never seen such a claim backed up
by real profiling data.

Jim

PS. Can you tell I've done this a few times?
Here are some of the projects that have endured this janitorial work:
  git
  gcc
  emacs
  glibc
  gnulib
  coreutils
  freeIPA
  libvirt
  util-linux-ng
  idutils
  openais
  corosync

PPS, Once clean, several of the above projects started using a
make rule (we call them syntax-check rules) to ensure that no
additional if-before-<free-like-function> is introduced.
If there's interest, I can add that, too.