Created attachment 134154 [details] [review]
Allow-nested-parameterized-types-for-GList-GSList-and-GHash.patch

Comment on attachment 134154 [details] [review]
Allow-nested-parameterized-types-for-GList-GSList-and-GHash.patch

It used to be like this and we removed it in favor of using proper XML which all the advantages. It is not going to be put back in the GIR. A similar syntax would be good to have in the annotationparser though.

Created attachment 134277 [details] [review]
Parse-parameterized-types-in-annotations.patch

New version of the patch which adds support to giscanner (annotation parser), instead of to the GIR XML parser.

Created attachment 134284 [details] [review]
Parse-parameterized-types-in-annotations.patch

Update patch to fix a minor PEP8 style issue (spaces around list brackets).

Comment on attachment 134284 [details] [review]
Parse-parameterized-types-in-annotations.patch

>From 6e09b030a2515b46e2f9848d9cc8b90574bd716c Mon Sep 17 00:00:00 2001
>From: C. Scott Ananian <cscott@litl.com>
>Date: Fri, 8 May 2009 11:52:17 -0400
>Subject: [PATCH] Parse parameterized types (using <>) in annotations.
>
>You can now specify a nested parameterized type in annotations as
>(for example):
>  @param: (type GLib.HashTable<utf8,GLib.HashTable<utf,utf>>)
>or
>  @param: (element-type utf8 GLib.HashTable<utf,utf>)

This looks much better, it separates the annotation syntax from the GIR.


>+    def resolve(self, type_str, orig_node=None):

Should probably be marked as private.

>diff --git a/tests/everything/everything.c b/tests/everything/everything.c

>+/**
>+ * test_ghash_nested_everything_return:
>+ * Specify nested parameterized types directly with the (type ) annotation.
>+ *
>+ * Return value: (type GLib.HashTable<utf8,GLib.HashTable<utf8,utf8>>) (transfer full):
>+ */
>+GHashTable *

>+/**
>+ * test_ghash_nested_everything_return2:
>+ * Another way of specifying nested parameterized types: using the
>+ * element-type annotation.
>+ *
>+ * Return value: (element-type utf8 GLib.HashTable<utf8,utf8>) (transfer full):
>+ */
>+GHashTable *

I am not so sure about the syntax here.

It is a bit confusing to have both () and <> in the annotation syntax.
The same thing could for instance be specified as:
  (element-type utf8 (type GLib.HashTable) (element-type utf8 utf8)) (transfer full):

But OTOH that is more to type and the gtk-doc usually lack space.

Colin: any thoughts or suggestions here?

Created attachment 134402 [details] [review]
Parse-parameterized-types-in-annotations.patch

Made resolve() method private.

I guess a highlevel question is - are we doing nested container types as a one-off for an odd NM method, or is the intent to fully support them everywhere?

In terms of syntax maybe

(element-type utf-8 (element-type GLib.HashTable utf-8 utf-8)) 

So in nesting cases we require the container type to come first.

My patches support nested container types everywhere.  I'll note that the annotation parser is currently not very smart: using a syntax involving parens will require it to get much smarter, since it's not counting nesting levels at all.  And (element-type utf8 (element-type ...)) is semantically incorrect; it should be:
  (element-type utf8 (type GLib.HashTable utf8 utf8))
or
  (type GLib.HashTable utf8 (type GLib.HashTable utf8 utf8))
ie, element-type presupposes a type annotation, which is only valid at top-level.

And this way lies madness.  If you wanted to be strictly consistent, you'd use:
  (type GLib.HashTable (type utf8) (type GLib.HashTable (type utf8) (type utf8)))
which is, of course, how it looks in the XML -- but I think humans would much prefer less typing.

The test in bug 581695 can be enabled if/when this patch is committed.

Ping!  The patch is lonely. 

Looking over this again, I agree with Johan; I like the "generics" syntax for annoations, but not in the .gir.  This patch doesn't do recursive nesting in the .gir, or does it?

Nope, the <> syntax is in annotations only; the GIR is proper XML nested tags.  (Sorry, the initial bug summary has drifted away from the patch.)

So is this patch cleared to commit, since the <> syntax is confined to annotations?

Sorry for dragging my feet on this one, I'm not really happy with the typelib side (bug 581686) mostly.  Let's go ahead and put this in though under the assumption that we will support nested types in some form.

On this patch though, I'd prefer if the parsing functions for nested types weren't themselves nested python functions =) Maybe move grab_one to be a toplevel function parse_nested_type or something?  I'm not sure why the function takes other functions as parameters when the same functions are always passed?

Well, the combiner parameter changes if you read carefully: the topmost "combiner" tries to recycle a given base type object, which preserves additional annotation information already present in the type object, but this can't be done for lower 'combiners'.  So we receive 'top_combiner, combiner' but always pass in 'combiner, combiner' to sub-invocations.

I don't strictly speaking have to make resolver() a passed-in function, but (in my mind) it helped me separate out the generic parsing code from the transformer-specific resolution code.  grab_one is really doing parsing only, and delegating all the semantic actions to resolver/combiner.

I seem to have a fixation on keeping namespaces clean, so I like to use nested functions if the functionality has no reason to escape its lexical context.  That's what they're in the language for, after all.  I'm just decomposing a pretty hairy parsing/resolution function into separate 'parse' and 'resolve' pieces for better readability.

Ah, ok.  Yeah, I understand the reasons for nesting, it just seems not unlikely to me that at some point some other bit of code will need to parse these types.  It's fine though to split it out at that time though.

My main concern is figuring out the typelib implications.

Typelib implications?  The typelib already supports nested <type> elements.  This patch is all you need.  The code to use these nested types in gjs is already present as well.

Pushed the patch and closing the bug; colin reopen this (or a different bug) if there are still typelib issues you're concerned about.

[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]