Re: __attribute__ ((cleanup) patch

[Dan Williams <dcbw redhat com>]

On Thu, 2012-10-18 at 10:19 -0400, Colin Walters wrote:
> Hi,
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=685440
> 
> has a patch which just landed, but I wanted to give wider discussion to
> this, because it's a very important infrastructural change.
> 
> First, one thing that came up is a concern about a GCC hard dependency.
> My understanding is that LLVM implements this too.  For compilers which
> implement C++, this is easy to support.  And since NetworkManager is
> highly tied to Linux, it's not like we have to care about MSVC.

LLVM is interesting, but until it (a) gets better adoption for major
projects, and (b) can compile NetworkManager with very minimal changes
to the NM codebase (ie, few if any #ifdefs), LLVM support is not a major
goal of NetworkManager.  It may be a goal in the future; and thus it
would be nice to know if LLVM builds fine with these extensions.

> Now, I plan to do a followup patch pretty soon which would let us
> get rid of a *lot* of g_free()/g_object_unref() calls.  By a
> conservative estimate, around 1500 lines.

Yeah; I write that code all the time and every time I do, I wish I could
use the attribute stuff.

> I've been using libgsystem cleanup extensively in several projects,
> and it's been a massive win.
> 
> Other examples of projects using this in C are systemd and upstart.
> 
> The downside is that this is kind of a one-way door.  Once done, it'd be
> immensely tedious to try to go back and add them again.  However, the
> benefit to the code is huge, and even better - trust me, it makes
> writing C fun again =)

Yes, that's the major downside.  The other potential issues are:

1) developer errors by choosing the wrong variable type; unless they are
somehow type-safe.  This is more of an education problem than anything
else

2) educating everyone who touches the code about the new method

Personally, I consider those as fixable issues, and outweighed by the
benefit of having the variables automatically freed, which reduces
memory leaks in NetworkManager, and makes the code more readable.

Dan