Coding practices proposals

[Greg Hudson]

On Thu, 2011-03-17 at 18:48 -0400, Sam Hartman wrote:
> My assumption is that you want it to be easy to make sure that the value
> is freed.

If a function has all of its owner pointers initialized to NULL at the
top and all of its cleanup calls at the bottom, and has no direct
returns after the first allocation, that makes it easier to verify that
the function has no leaks and no double frees.

One obvious exception is when you allocate temporary memory within a
loop body (but the body is still too small to warrant being split out
into a helper function).  In that case, you clearly must free it within
the loop body.

The situations you describe are cases where it's safe to free a variable
early, but in only one of them is it really advantageous to do so:

> * If a pointer is going out of scope it seems reasonable to free. I
>   don't think we want to force things into top most scope just to clean
>   them up in a particular place

However, we have a recommendation against declaring variables in inner
scopes (which I didn't write):

http://k5wiki.kerberos.org/wiki/Coding_style/Practices#Local_variables

That may be one of the more controversial documented practices.
Personally, I think grouping cleanup code together is compelling enough
to argue against inner scope declarations for owner pointers.

I wrote:
>> 3. Initialize variables containing owner pointers to NULL.  Free them
>> in the function's cleanup handler, even if it would be possible to
>> free them sooner.

Ken wrote:
> If you take "free them" to mean "call the appropriate 'free'
> function", as I did on first reading, this means, "even if in some
> code paths you do free them sooner".  But if you take "free them" to
> mean "perform the call that actually releases the storage", then the
> above description seems to discourage doing it any sooner, "even if it
> would be possible".  I think the former is the better approach.

I don't understand the difference between "calling the appropriate free
function" and "performing the call that actually releases the storage".

What I've seen problems with is code like:

  ret = allocate_thing(&ptr);
  if (ret)
    goto cleanup;
  ret = this_might_fail(...);  /* This call added in later */
  if (ret)
    goto cleanup;
  ret = this_might_fail_too(..., ptr, ...);
  free_thing(ptr);  /* Done here because nothing uses ptr after this */
  if (ret)
    goto cleanup;
  ...
cleanup:
  free_other_stuff(other_stuff);
  return ret;

Because the original coder wanted to free ptr as early as possible,
there is no good way to handle the error from this_might_fail() when it
is added.  Although there wasn't anything unsafe about the early free
when the function was originally written, it became an issue as the
function grew more complicated.  And there was never a substantial
advantage to freeing it sooner; the reduction in maximum heap size from
aggressively freeing stuff is generally negligible.

Ken also wrote:
> ... or being freed?

Yes, definitely.  I was thinking of that as a form of going out of
scope, but technically it isn't.