GNOME Bugzilla – Bug 744747
Add g_autofree
Last modified: 2015-02-23 10:24:33 UTC
The g_autoptr() being associated with the type name works out really well for things like GHashTable. However, it's a bit more awkward to associate with "gchar". Also because one can't use "char". Similarly, there are a lot of other "bare primitive array" types that one might reasonably use. This patch does not remove the autoptr for "gchar", even though I think it's rather awkward and strange. Also while we're here, add a test case for the cleanup bits.
Created attachment 297141 [details] [review] Add g_autofree
This is a follow up from https://bugzilla.gnome.org/show_bug.cgi?id=743640
my main counterargument, copied from the other bug: I'm just not sure this is needed right now. It was never the intention that g_autoptr() would work with absolutely everything out of the box. This is why G_DEFINE_AUTOPTR_CLEANUP_FUNC() is a public API. It is intended that people use this. I've been writing some code that makes use of sqlite lately, and I don't find it strange at all that I should have to write G_DEFINE_AUTOPTR_CLEANUP_FUNC(sqlite3_stmt, sqlite3_finalize) G_DEFINE_AUTOPTR_CLEANUP_FUNC(sqlite3, sqlite3_close_v2) in order to be able to use the macros... It is intended that people proceed like this, in a typesafe way, rather than introducing an adhoc macro that can cover all of the cases, including the wrong ones. ::: glib/glib-autocleanups.h @@ +23,3 @@ +static inline void +g_autoptr_cleanup_generic_gfree (void *p) I'm very concerned about the lack of typesafety here. In theory this would work even for an integer.
All of that said, I am at least partially convinced by your argument that g_autoptr(gchar) is pretty weird.
(In reply to Ryan Lortie (desrt) from comment #55) > I've been writing some code that makes use of sqlite lately, and I don't > find it strange at all that I should have to write > > G_DEFINE_AUTOPTR_CLEANUP_FUNC(sqlite3_stmt, sqlite3_finalize) > G_DEFINE_AUTOPTR_CLEANUP_FUNC(sqlite3, sqlite3_close_v2) I agree - the autoptr works well for those! This patch is an additional option for things that are freed with {g_,}free. If you really want I can keep the define for "gchar". > I'm very concerned about the lack of typesafety here. In theory this would > work even for an integer. Well, this is C - there's a lot of non-typesafe things. I mean, we have g_object_unref() take a gpointer too. A number of codebases quite successfully use "cleanup attribute style" - systemd, ostree, librepo, PackageKit, etc. Again, I'm not arguing against autoptr, but for also having the attribute style for the very massive common case of "free()" (and possibly gobject). Remember I've been using cleanup macros a *lot* over the past few years. [reply] [−] Comment 57 Colin Walters [developer] 2015-02-15 13:02:33 EST The other aspect to this is that g_autoptr(GVariant) and g_autoptr(gchar) are fundamentally different in that GVariant is *always* heap allocated. So conceptually, the "*" is visually redundant. But there's a very important difference between "char" and "char*" obviously. And it's not uncommon to see both. Having the '*' "hidden" by g_autoptr breaks my visual scan. I have to stop and think. Whereas when I see "GVariant" I just know it's a pointer, I don't even need to parse the "ptr" name in the macro. [reply] [−] Comment 60 Richard Hughes [developer] 2015-02-17 14:36:40 EST Ya, _cleanup_free_ means that I can do something like: _cleanup_free_ guint8 *buffer = g_new0(guint8, 256); ...which I use a *lot*. I think we need both styles, really.
We're quite late in the cycle, and this is blocking a full migration to GLib autocleanups for my projects. It's not the end of the world, I can keep using libglnx, but there are other projects that currently have their own copies (libhif, librepo, etc.) and I'd like to only migrate once.
I'm sort of waiting for a better proposal (in terms of API) to pop out... Part of the reason that I didn't like this stuff in the first place was on account of the API issues (mentioned above) and it wasn't until Alex and I had a chat and came to a better solution that I was convinced it was palatable enough to include in GLib. My opinion there hasn't substantially changed...
(In reply to Ryan Lortie (desrt) from comment #7) > Part of the reason that I didn't like this stuff in the first place was on > account of the API issues (mentioned above) and it wasn't until Alex and I > had a chat and came to a better solution that I was convinced it was > palatable enough to include in GLib. My opinion there hasn't substantially > changed... I'm not aware of a project using the cleanup macros that does it any better. Keep in mind too that many of these other projects are quite successfully using it and they clearly find it palatable. Finally, if we find a better API for the generic free case, adding this certainly wouldn't block a new API later.
Review of attachment 297141 [details] [review]: Your argument for being able to port to GLib's version of these macros is pretty convincing, and ya -- we can fix this later if we come up with something more clever. I'll try to get the ghandle stuff done up soon. ::: glib/docs.c @@ +2434,3 @@ + * + * This means it's useful for any type that is returned from + * g_malloc() (or system malloc if g_mem_is_system_malloc()). Please remove this mention -- we really should not be encouraging people to mix calls like this. ::: glib/glib-autocleanups.h @@ +26,3 @@ +{ + void **pp = (void**)p; + g_free (*pp); Maybe worth adding a check here for if (*pp != NULL) before calling g_free(). Since this is an inline, the compiler will often know if a call is never necessary (for example: variable not used yet, after g_steal_pointer, etc.).
btw: this is going to happen, and it's going to be confusing... x.c: In function ‘main’: x.c:8:26: error: ‘table’ undeclared (first use in this function) g_autofree(GHashTable) table = NULL; ^ x.c:8:26: note: each undeclared identifier is reported only once for each function it appears in
> Maybe worth adding a check here for if (*pp != NULL) before calling > g_free(). Since this is an inline, the compiler will often know if a call > is never necessary (for example: variable not used yet, after > g_steal_pointer, etc.). Is expanding many call sites with a conditional cheaper than having them do a call to g_free() which already tests for NULL? My instinct is that if your program is at a state where that matters either way, you're probably doing "named goto cleanups" like Linux does, not using __attribute__() cleanup. But I'm not enough of an expert to say. Although hmm, I just noticed g_free() is using G_LIKELY()...and I remember reading that missed annotated branch predictions can be quite expensive. If we're pervasively using cleanup macros there'll be a lot more calls to g_free() on NULL potentially. Perhaps that argues more for including the inline NULL check now? Side note: I think it might be nice if GLib offered -DGMEM_IS_SYSTEM_MALLOC or for app authors to avoid the indirection through GLib, particularly now that you can't actually use g_mem_set_vtable() in anything that links to GObject now that we use the __attribute__((constructor)). (In reply to Ryan Lortie (desrt) from comment #10) > x.c:8:26: error: ‘table’ undeclared (first use in this function) > g_autofree(GHashTable) table = NULL; > ^ > x.c:8:26: note: each undeclared identifier is reported only once for each > function it appears in Yeah, but at least that one is a compile error. Do you want to remove the g_autoptr(gchar) or keep it?
Attachment 297141 [details] pushed as d0105f1 - Add g_autofree
Committed with the system malloc part removed (although, see my followup comment) as well as adding the conditional in the cleanup function, and test cases for NULL.
(In reply to Colin Walters from comment #11) > Do you want to remove the g_autoptr(gchar) or keep it? Let's kill it. We can put it back later if we want.