Re: __attribute__ ((cleanup) patch

[Dan Williams <dcbw redhat com>]

On Thu, 2012-10-18 at 11:19 -0400, Pavel Simerda wrote:
> > From: "Colin Walters" <walters verbum org>
> > Hi,
> 
> Hi, thanks for a quick reply.
> 
> > 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.
> 
> Yes.
> 
> > My understanding is that LLVM implements this too.
> 
> We have been too conservative to adopt the C99 standard and now we
> are brave enough to go for non-standard compiler features.

We already use some C99 features; basic stuff like __func__ and
snprintf().  I don't have a problem with using "-std=c99" to build
stuff, but it seems like either GCC is already exposing those features
with -std=gnu89.  The one thing I *don't* like in c99 is the mixed
declarations and code; that clutters up the code horribly.

> > For compilers which implement C++, this is easy to support.
> 
> I don't deny this, but this is not how the question stands, currently.
> 
> > And since NetworkManager is highly tied to Linux
> 
> NetworkManager is tied to Linux, now, but in my opinion not as highly
> as it might seem at the first glance. Dan might disagree.

It is not a goal of NetworkManager to actively support other platforms.
If there was a nice, easy way to support *BSD (and with the platform
stuff maybe there would be) then we'd entertain the patch, but we have
enough things to work on at the moment than BSD support.

> > it's not like we have to care about MSVC.
> 
> Nobody's talking about MSVC here.
> 
> > Now, I plan to do a followup patch pretty soon which would let us
> > get rid of a *lot* of g_free()
> 
> You can get rid of g_free's on small strings for free with a pretty
> standard way through g_alloca, for example. For that, this is overkill.

That doesn't work for allocated values passed back from other functions,
which happens quite a bit.  But the libgsystem stuff does work for that
AFAIK.  ie:

char *foo;
call_some_func (&foo);
do_something
g_free (foo);

cannot be made simpler with g_alloca().

> > /g_object_unref() calls.
> 
> For this, I would need some more detailed information. This is just not enough
> for merging someone's experiments that don't even have any documentation. I admit
> it is tempting to use a real important project for one's experiments but it might
> not be the right direction for the project.

I don't consider this an experiment at this point.  We're unlikely to
convert to C++ any time soon, and anything that makes C easier to code,
reduces programmer errors and memory leaks, and cleans up the existing
code is a win, as long as it's not too complicated.  Given that this
stuff is *less* complicated than the existing code, it's a win.  Yes,
there is a bit of education required, but perhaps we could also work on
making some things compile-time errors instead of runtime gotchas.

> > conservative estimate, around 1500 lines.
> > I've been using libgsystem cleanup extensively in several projects,
> > and it's been a massive win.
> 
> Then why it wasn't good enough for Glib but is good enough for NetworkManager? Why
> I don't see a link to elaborate explanation and documentation? Success stories. More
> information?
> 
> > 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.
> 
> That's exactly the problem I have with such quick inclusions without a proper discussion.
> 
> > However, the benefit to the code is huge,
> 
> I'm not yet even convinced about this because of total lack of documentation to be
> found right away. And I'm not convinced that switching to a non-standard programming
> language is the price to pay.
> 
> > and even better - trust me, it makes writing C fun again =)
> 
> You mean writing a language incompatible with the C standards :). Changing
> the programming language for the whole project in a forward-only compatible
> manner is a big decision and I'm not really happy with the way it has been
> done.

We use GCC.  We don't use obscure features of GCC.  We don't adhere
blindly to C standards, we deviate where it makes sense.  Yes, this
issue is up for discussion, but I also haven't heard anything that makes
me think a revert is in order.

Dan

> So, I haven't changed my mind. I propose the following path:
> 
> 1) Revert
> 
> 2) Document including the reasons why this cannot be added to Glib but should be
> added to NetworkManager
> 
> 3) Discuss
> 
> 4) Decide
> 
> I don't think this should be a one-week process, so I personally insist on starting
> with the step (1). I would like to discourage this practice for future decision
> making regarding such huge one-way changes.
> 
> Cheers,
> 
> Pavel
> _______________________________________________
> networkmanager-list mailing list
> networkmanager-list gnome org
> https://mail.gnome.org/mailman/listinfo/networkmanager-list