After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 791342 - Add autoptr support for lists
Add autoptr support for lists
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: glist
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2017-12-07 14:14 UTC by Alexander Larsson
Modified: 2017-12-28 08:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Example patch (untested) (2.11 KB, patch)
2017-12-07 14:15 UTC, Alexander Larsson
needs-work Details | Review
Add support for g_auto[s]list(Type) (8.05 KB, patch)
2017-12-19 10:18 UTC, Alexander Larsson
accepted-commit_now Details | Review
Do not expand autoptr macros when running introspection (1.71 KB, patch)
2017-12-22 15:07 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review

Description Alexander Larsson 2017-12-07 14:14:08 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;
Comment 1 Alexander Larsson 2017-12-07 14:15:45 UTC
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.
Comment 2 Philip Withnall 2017-12-07 15:03:18 UTC
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
Comment 3 Alexander Larsson 2017-12-19 10:18:19 UTC
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.
Comment 4 Philip Withnall 2017-12-21 10:42:33 UTC
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.
Comment 5 Alexander Larsson 2017-12-21 15:13:22 UTC
Attachment 365737 [details] pushed as f49a93b - Add support for g_auto[s]list(Type)
Comment 6 Emmanuele Bassi (:ebassi) 2017-12-22 13:29:00 UTC
This broke the build for every project generating Vala bindings because of bug 629682 in gobject-introspection.
Comment 7 Emmanuele Bassi (:ebassi) 2017-12-22 13:35:56 UTC
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.
Comment 8 Philip Withnall 2017-12-22 14:45:02 UTC
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?
Comment 9 Emmanuele Bassi (:ebassi) 2017-12-22 14:48:27 UTC
(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.
Comment 10 Philip Withnall 2017-12-22 15:06:48 UTC
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.
Comment 11 Emmanuele Bassi (:ebassi) 2017-12-22 15:07:48 UTC
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.
Comment 12 Philip Withnall 2017-12-22 15:10:48 UTC
Review of attachment 365881 [details] [review]:

++
Comment 13 Emmanuele Bassi (:ebassi) 2017-12-22 15:12:42 UTC
Attachment 365881 [details] pushed as a07b578 - Do not expand autoptr macros when running introspection
Comment 14 Emmanuele Bassi (:ebassi) 2017-12-28 08:08:12 UTC
Another fun build breakage caused by this: https://github.com/flatpak/flatpak/issues/1279
Comment 15 Emmanuele Bassi (:ebassi) 2017-12-28 08:08:48 UTC
Not strictly speaking something caused by this: I'm surprised it ever worked in the first place.