GNOME Bugzilla – Bug 791342
Add autoptr support for lists
Last modified: 2017-12-28 08:08:48 UTC
Having an autoptr version for lists that does deep freeing would be very nice. We can't do generic container support, but since GList is so common we could do special-cased support of something like: g_autolist(SomeType) list = NULL;
Created attachment 365195 [details] [review] Example patch (untested) This patch demos the approach. I didn't test it, but i believe it will work and am mostly interested in discussing if we want this at all.
Review of attachment 365195 [details] [review]: Yes, that looks like a good idea to me. Moving forward, we’d need: • Documentation • A g_autoslist for GSList • Unit tests
Created attachment 365737 [details] [review] Add support for g_auto[s]list(Type) This lets you do g_autoptr style cleanup of GList that does deep freeing.
Review of attachment 365737 [details] [review]: Looks good to push with this one nitpick fix, thanks. ::: glib/tests/autoptr.c @@ +415,3 @@ +{ + gboolean *freed = ptr; + *freed = TRUE;; Nitpick: Double semicolon.
Attachment 365737 [details] pushed as f49a93b - Add support for g_auto[s]list(Type)
This broke the build for every project generating Vala bindings because of bug 629682 in gobject-introspection.
I'm a bit conflicted; on one side, I really want this to land. On the other, the introspection bug has been open since 2012, and we kind of rely on the current behaviour; additionally, this landed in the period of the year when a lot of people are on vacation, which reduces the chances of a timely fix. This alone would be grounds for a revert. I'll let Philip decide, but I'm going to re-open this bug anyway.
Ufff, that’s a pain. From a failed build log (http://build.gnome.org/continuous/buildmaster/builds/2017/12/22/0/build/log-libdazzle.txt): FAILED: src/libdazzle-1.0.vapi /usr/bin/vapigen --quiet --library=libdazzle-1.0 --directory=/ostbuild/source/libdazzle/_build/src --metadatadir=/ostbuild/source/libdazzle/src --pkg=gio-2.0 --pkg=gtk+-3.0 --metadatadir=/ostbuild/source/libdazzle/src /ostbuild/source/libdazzle/_build/src/Dazzle-1.0.gir Dazzle-1.0.gir:18.39-18.39: error: too few type arguments And from that GIR file (http://build.gnome.org/continuous/buildmaster/builds/2017/12/22/0/build/tmp-libdazzle_x86_64/libdazzle/_build/src/Dazzle-1.0.gir): <alias name="PatternSpec_listautoptr" c:type="DzlPatternSpec_listautoptr"> <type name="GLib.List" c:type="GList*"/> </alias> (line 18 is the <type/>) --- Since the g_autolist() types are only used in function implementations, not in any arguments or return types, it should be safe to just elide them from the GIR entirely. That could be possible by adding a hack in g-ir-scanner, or by wrapping stuff in gmacros.h in #ifndef __GI_SCANNER__, I think? Emmanuele, you said on IRC you’d tried a fix — what did you try?
(In reply to Philip Withnall from comment #8) > Ufff, that’s a pain. From a failed build log > (http://build.gnome.org/continuous/buildmaster/builds/2017/12/22/0/build/log- > libdazzle.txt): > > FAILED: src/libdazzle-1.0.vapi > /usr/bin/vapigen --quiet --library=libdazzle-1.0 > --directory=/ostbuild/source/libdazzle/_build/src > --metadatadir=/ostbuild/source/libdazzle/src --pkg=gio-2.0 --pkg=gtk+-3.0 > --metadatadir=/ostbuild/source/libdazzle/src > /ostbuild/source/libdazzle/_build/src/Dazzle-1.0.gir > Dazzle-1.0.gir:18.39-18.39: error: too few type arguments > > And from that GIR file > (http://build.gnome.org/continuous/buildmaster/builds/2017/12/22/0/build/tmp- > libdazzle_x86_64/libdazzle/_build/src/Dazzle-1.0.gir): > > <alias name="PatternSpec_listautoptr" > c:type="DzlPatternSpec_listautoptr"> > <type name="GLib.List" c:type="GList*"/> > </alias> > > (line 18 is the <type/>) Yep. It's missing the type of the element — basically, g-ir-scanner is emitting invalid introspection data because it doesn't really know how to handle aliases of container types. > --- > > Since the g_autolist() types are only used in function implementations, not > in any arguments or return types, it should be safe to just elide them from > the GIR entirely. That could be possible by adding a hack in g-ir-scanner, > or by wrapping stuff in gmacros.h in #ifndef __GI_SCANNER__, I think? That may work, yes. > Emmanuele, you said on IRC you’d tried a fix — what did you try? I've tried fixing the introspection scanner to mark aliases of container types without element-type annotation as not introspectable, but that broke the introspection test suite because we do expect them to be valid, or at least manageable.
From a discussion on IRC: • We’ll work around this now by adding #ifndef __GI_SCANNER__ to gmacros.h. • This is arguably something we should have always had anyway, rather than a workaround. The autoptr aliases should never have been introspectable. • Arguably, vapigen aborting on an incomplete type in an alias is a bug in Vala, since apparently the gobject-introspection unit tests expect this to be supported (as per the last paragraph of Emmanuele’s comment #9). • Emmanuele might look at fixing bug #629682 properly, but let’s pick this up properly in January.
Created attachment 365881 [details] [review] Do not expand autoptr macros when running introspection The introspection scanner chokes fairly badly on the types we create, and that got even worse when the autolist support landed. Now, every time we declare a new GObject type we automatically get incomplete aliases to container types that gobject-introspection and Vala do not know how to handle. Since the autoptr machinery is not really introspectable to begin with, as it's a C utility extension that depends on the C compiler being used to compile a C project that depends on GLib, we can mark the whole section as non-introspectable using the __GI_SCANNER__ pre-processor symbol.
Review of attachment 365881 [details] [review]: ++
Attachment 365881 [details] pushed as a07b578 - Do not expand autoptr macros when running introspection
Another fun build breakage caused by this: https://github.com/flatpak/flatpak/issues/1279
Not strictly speaking something caused by this: I'm surprised it ever worked in the first place.