Created attachment 168880 [details] [review]
scanner: If GI_SCANNER_DEBUG is set, drop into a debugger on error

Useful when debugging build problems for both me, and potentially over
IRC.

Created attachment 168881 [details] [review]
.gitignore: Update

Created attachment 168882 [details] [review]
gobject-introspection-1.0.pc: Export datadir

This will be used for consumers to find regress.[ch].

Created attachment 168883 [details] [review]
tests: Remove GIMarshallingTests, rename Everything to Regress

GIMarshallingTests will be replaced with automatically
generated code; it was far from comprehensive, and was a pain
to maintain.

The namespacing in Everything was broken; it had an empty C
prefix effectively, because all the symbols just started with
"test".  We do want "test" as part of the symbols, since otherwise
the exported API would be weird.  Fix this by changing the namespace
to Regress (i.e. prefixing all the C code with Regress).  This
makes sense anyways because Everything wasn't really Everything.

We no longer install a pre-built regress.(so,gir,typelib); instead we
install the C code to
$(datadir)/gobject-introspection/tests/regress.[ch].
Bindings should compile this.

Created attachment 168884 [details] [review]
tests: Fix namespacing for BarApp (should be just Bar)

Created attachment 168885 [details] [review]
tests: Fix namespacing for Foo, Drawable, Utility, TestInherit

Various tests had incorrect namespacing; the scanner will be
more strict about this in the future.

Created attachment 168886 [details] [review]
tests: Fix namespacing for offsets

Created attachment 168887 [details] [review]
Move alias target to <type>

This makes type parsing more uniform.

Delete the typedef for GSList in foo.h - that's not
supported anymore, or at least for now.

Conflicts:

	tests/scanner/Foo-1.0-expected.gir
	tests/scanner/Utility-1.0-expected.gir

Created attachment 168888 [details] [review]
Use GLib types consistently

Rather than have the scanner/parser handle both e.g. "glong" and
"long", simply use the GLib types everywhere.

This commit adds TYPE_LONG_LONG and TYPE_LONG_DOUBLE to the
scanner types; however, rather than add them to the typelib,
they're just marked as not-introspectable.

Created attachment 168889 [details] [review]
tests/scanner: Update annotations and tests

First of all, add missing (transfer) annotations that will be
required by the new scanner.

Other changes:
Don't use the (type bitfield) hack; the new scanner will not accept it.
Use shifts in the flag constants instead.

Use typedefs consistently for structures.

Drop scanning of anonymous structure/union members.

Created attachment 168890 [details] [review]
Major rewrite

One of the first big changes in this rewrite is changing the Type
object to have separate target_fundamental and target_giname properties,
rather than just being strings.  Previously in the scanner, it was
awful because we used heuristics around strings.

The ast.py is refactored so that not everything is a Node - that
was a rather useless abstraction.  Now, only things which can have
a GIName are Node.  E.g. Type and Field are no longer Node.

More things were merged from glibast.py into ast.py, since it isn't
a very useful split.

transformer.py gains more intelligence and will e.g. turn GLib.List
into a List() object earlier.  The namespace processing is a lot
cleaner now; since we parse the included .girs, we know the C
prefix for each namespace, and have functions to parse both
C type names (GtkFooBar) and symbols gtk_foo_bar into their
symbols cleanly.  Type resolution is much, much saner because
we know Type(target_giname=Gtk.Foo) maps to the namespace Gtk.

glibtransformer.py now just handles the XML processing from the dump,
and a few miscellaneous things.

The major heavy lifting now lives in primarytransformer.py, which
is a combination of most of annotationparser.py and half of
glibtransformer.py.

annotationparser.py now literally just parses annotations; it's
no longer in the business of e.g. guessing transfer too.

finaltransformer.py is a new file which does post-analysis for
"introspectability" mainly.

girparser.c is fixed for some introspectable=0 processing.

Created attachment 168891 [details] [review]
tests: Readd gimarshallingtests.[ch]

For backwards compatibility with tests, for now resurrect these
test cases (also, fix the namespacing on them while we're here).

Like Everything, these get installed in source form.

Created attachment 168892 [details] [review]
tests: Add --includedir for g-ir-compiler

Created attachment 168893 [details] [review]
tests: Some more fixes to build uninstalled

Created attachment 168894 [details] [review]
gimarshallingtests: Fix (inout) test cases, drop unsupported API

First, (inout) doesn't mean "mutates the argument".  It means "I take an
input argument here, and will also return a *new* argument in this position."

C API which mutates e.g. structures is just unannotated.  Mutation of
containers like GList, GHashTable is simply disallowed.

Secondly, we can't support C API which returns non-boxed structures with
a transfer other than (none).  The scanner will warn about this in the future.

Created attachment 168895 [details] [review]
major: Fix parent for GInitiallyUnowned

Created attachment 168896 [details] [review]
scanner: Avoid internal invalid Type instances from parents

We were adding a trailing ',' in the parent string, clean that up; and
don't attempt to create a Type from the empty string.

Created attachment 168897 [details] [review]
scanner: Better handling of empty namespace prefix for X

Add namespacing prefixes to the static .gir files.  Support the empty
prefix, as is needed for xlib.

Created attachment 168898 [details] [review]
scanner: Add a hack for AtkAttributeSet

Created attachment 168899 [details] [review]
scanner: Filter interface prerequisites and class implements for unknown types

This works around the hidden GtkFileChooserEmbed interface of GtkFileChooserWidget.

Created attachment 168900 [details] [review]
configure.ac: Bump version to 0.9.5

Review of attachment 168880 [details] [review]:

Rest looks good

::: tools/g-ir-scanner.in
@@ +25,3 @@
+if 'GI_SCANNER_DEBUG' in os.environ:
+    def on_exception(type, value, tb):
+        import pdb; pdb.pm()

No reason to use ; here, just split it on separate lines.

Review of attachment 168881 [details] [review]:

Yes

Review of attachment 168882 [details] [review]:

Yes, do we need to update introspection.m4 as well, so it gets pulled in automatically?

Review of attachment 168883 [details] [review]:

Yes, as long as at least pygobject and gjs gets updated.
If possible try to do this after landing the rest of the scanner changes.

Review of attachment 168885 [details] [review]:

Yes, this is great.

Review of attachment 168886 [details] [review]:

Yes. Perhaps squash all the 'incorrect namespace' patches together

Review of attachment 168887 [details] [review]:

Remove conflicts from commit message?

::: girepository/girparser.c
@@ +3131,3 @@
       if (require_end_element (context, ctx, "alias", element_name, error))
 	{
+	  g_free (ctx->current_alias);

Is this freed in the error paths as well?

Review of attachment 168888 [details] [review]:

This is unfortunate, if we're really going to reduce the types that can be used we
should use the canonical non-GLib types everwhere, eg. long instead of glong, uint8 instead of guint8.
Eg, seems like upstream GLib based libraries are moving from using gchar to char etc.

TL;DR patch

Review of attachment 168889 [details] [review]:

Seems okay, but just a bunch of random changes grouped together, could be split up I guess.

::: tests/scanner/foo.h
@@ +299,3 @@
 };
 
+typedef union _FooUnion FooUnion;

This will probably break stuff in Gtk/Gdk if it's no longer supported.

Review of attachment 168891 [details] [review]:

Can't be squashed with the patch that removes them?

Review of attachment 168892 [details] [review]:

This is the wrong location I think for this kind of change, there's a toplevel make file which provide flags
for the scanner and the compiler

Review of attachment 168893 [details] [review]:

Okay, more in line with the previous patch, squash?

Review of attachment 168894 [details] [review]:

Okay, TL;DR but make sure the pygobject guys are aware/happy about this

Review of attachment 168895 [details] [review]:

Yes

Review of attachment 168896 [details] [review]:

Yes

Review of attachment 168897 [details] [review]:

Could perhaps separate this into two commits, one to update the girs and one to update the code.

::: giscanner/primarytransformer.py
@@ +97,3 @@
         if cls is supercls:
             return True
+        if cls.parent and (not cls.parent.target_giname == 'GObject.Object'):

not x == y -> x != y
remove parenthesis as well

@@ +626,3 @@
             resolved_parent = None
+            if node.parent_chain:
+                for parent in node.parent_chain:

make node.parent_chain an empty list and kill the if,

for x in []:
   A
else:
   B

will execute B, so it should be fine.

::: giscanner/scannermain.py
@@ +264,3 @@
+    # None; if the user didn't specify the options, we want to use None so
+    # the Namespace constructor picks the defaults.
+    if len(options.identifier_prefixes) == 0:

if not options.identifier_prefixes ?
Even better, invert it:

if options.identifier_prefixes:
   identifier_prefixes = options.identifier_prefixes
else:
   identifier_prefixes = None

@@ +268,3 @@
+    else:
+        identifier_prefixes = options.identifier_prefixes
+    if len(options.symbol_prefixes) == 0:

Ditto see above

::: giscanner/transformer.py
@@ +281,3 @@
         matches = []
         for ns in self._iter_namespaces():
+            if ns.identifier_prefixes:

Make it an empty list by default and kill the if

@@ +784,3 @@
+                target = namespace.get_by_ctype(pointer_stripped)
+            if target:
+                typeval.target_giname='%s.%s' % (namespace.name, target.name)

Missing spaces around =

Review of attachment 168898 [details] [review]:

::: giscanner/primarytransformer.py
@@ +47,2 @@
     def transform(self):
+        # Dirty hack for now...maybe eventually we'll support the "typedef GSList FooSet"

So this is the typedef breakage, how much work is it to support it.
I'm pretty sure that people outside of Atk/Pango/Gtk uses this somewhere.
Would be unfortunate to regress that, which projects have you tested this against?

@@ +51,3 @@
+            attribute = self._namespace.get('Attribute')
+            attributeset = self._namespace.get('AttributeSet')
+            if attribute and attributeset:

Not using the values, so:

if ('Attribute' in self._namespace and
    'AttributeSet' in self._namespace):
   ...

Review of attachment 168899 [details] [review]:

Rest looks good

::: giscanner/primarytransformer.py
@@ +618,3 @@
 
+    def _resolve_and_filter_type_list(self, typelist):
+        """Given a ilst of Type instances, return a new list of types with

ilst -> list

@@ +625,3 @@
+            resolved = self._transformer.resolve_type(typeval)
+            if not resolved:
+                failed.append(typeval)

Iterate over a copy instead and remove as you go.

for x in typelist[:]:
   ...
   if failed:
      typelib.remove(x)

Review of attachment 168900 [details] [review]:

Okay, depends on the other commits.

*** Bug 622609 has been marked as a duplicate of this bug. ***

(In reply to comment #24)
> Review of attachment 168882 [details] [review]:
> 
> Yes, do we need to update introspection.m4 as well, so it gets pulled in
> automatically?

Mmm, it's only used by bindings, not libraries or apps, so I don't see too much of a need for it to be automatic.

(In reply to comment #28)
> Review of attachment 168887 [details] [review]:
> 
> Remove conflicts from commit message?
> 
> ::: girepository/girparser.c
> @@ +3131,3 @@
>        if (require_end_element (context, ctx, "alias", element_name, error))
>      {
> +      g_free (ctx->current_alias);
> 
> Is this freed in the error paths as well?

No...but very little in girparser.c is.  We just abort the process if parsing fails anyways.

Review of attachment 168890 [details] [review]:

::: gir/DBus-1.0.gir
@@ +1,2 @@
 <?xml version="1.0"?>
+<repository version="1.2"

All gir changes look good, but should probably be moved to separate commit

::: girepository/girmodule.c
@@ +310,3 @@
   header = (Header *)data;
   memcpy (header, G_IR_MAGIC, 16);
+  header->major_version = 4;

Do we document the version changes somewhere?

::: giscanner/annotationparser.py
@@ +135,3 @@
         self._transformer = transformer
+        self._source_scanner = source_scanner
+        self._namespace = transformer.namespace

Is transformer/scanner/namespace still used, could simplify the API here by only sending in comments in parse() and whatever is actually used here.

::: giscanner/ast.py
@@ +32,3 @@
 """
 
+    resolved = property(lambda self: (self.target_fundamental or

Hard to read, please make it a real method:

@property
def resolved(self):
    return (...)

@@ +70,3 @@
+
+    def __cmp__(self, other):
+    resolved = property(lambda self: (self.target_fundamental or

Perhaps do an isinstance() to make sure we're not trying to compare against something different

@@ +86,3 @@
+        any."""
+        if isinstance(typeval, (list, tuple)):
+            for val in typeval:

return any(self.is_equiv(val) for val in typeval)

@@ +261,3 @@
 
+class Namespace(object):
+    names = property(lambda self: self._names)

I'd use methods here too, lambda is just too obscure.

@@ +272,3 @@
+        self.version = version
+        if identifier_prefixes:
+            self.identifier_prefixes = identifier_prefixes

identifier_prefixes should never be [], it simplifies other code

@@ +301,3 @@
+        return Type(target_giname=target, ctype=ctype)
+
+    def contains_ident(self, ident):

Be very careful about private/public methods, you *must* add a _ in the front if it's not used outside of the class itself.

@@ +304,3 @@
+        """Return True if this namespace should contain the given C
+identifier string."""
+        if not self.identifier_prefixes:

for instance, this whole method can be written as:

return any(ident.startswith(prefix) for prefix in self.identifier_prefixes)

@@ +386,3 @@
+
+    @classmethod
+    def from_string(self, string):

It's cls, not self.

@@ +387,3 @@
+    @classmethod
+    def from_string(self, string):
+        return Include(*string.split('-', 1))

cls instead of Include

@@ +391,3 @@
+    def __cmp__(self, other):
+        if not isinstance(other, Include):
+            return cmp(self, other)

Shouldn't this case raise a TypeError or just return 0 (or whatever means it's not equal)

@@ +398,3 @@
+
+    def __hash__(self):
+        return hash((self.name, self.version))

hash(str(self)) ?

@@ +468,1 @@
+    def _walk(self, callback, chain):

Unused, kill?

@@ +529,3 @@
+    def __init__(self, array_type, element_type, **kwargs):
+        Type.__init__(self, target_fundamental='<array>',
+                      **kwargs)

Please do not use **kwargs, it's too hard to use properly to avoid adding bugs here.
To use properly you need to validate that there are no unknown parameters sent in.

@@ +530,3 @@
+        Type.__init__(self, target_fundamental='<array>',
+                      **kwargs)
+        if array_type in (None, self.C):

never use == (or in which uses ==) against None, correct here is:
if (self.array_type is None or
    self.array_type == self.C):

 or so

::: giscanner/codegen.py
@@ +35,3 @@
+# -*- Mode: Python -*-
+        if (isinstance(param, ast.Parameter) and
+            param.direction in (ast.PARAM_DIRECTION_OUT,

move this logic to param?
param.is_out() -> direction in [out, inout]
param.is_in() -> direction in [in, inout]

as it will simplify a lot of code.

@@ +127,3 @@
+        self._codegen_start()
+
+        for node in self.namespace.itervalues():

I think __iter__ in namespace should just return the nodes, and this should thus be
for node in self.namespace:

::: giscanner/finaltransformer.py
@@ +21,3 @@
+from . import glibast
+
+class FinalTransformer(object):

FinalTransformer is not a great name, we should rename it to something which makes it easier to understand.

Seems to warn and mark things as introspected, so IntrospectableTransformer?

::: giscanner/girparser.py
@@ +103,3 @@
+
+    def _find_children(self, node, name):
+        result = []

maybe return [child.tag in node.children if tag.name]

Should probably be moved to Node and be called get_children_by_name()

@@ +205,3 @@
+        parent = node.attrib.get('parent')
+        if parent:
+            parent_type = self._namespace.type_from_name(parent)

make type_from_name(None) -> None to simplify some code here, right?

::: giscanner/girwriter.py
@@ +262,3 @@
+            if ntype.length_param_name is not None:
+                assert function
+                attrs.insert(0, ('length', '%d' \

no need for \ AFACT

::: giscanner/glibast.py
@@ +24,2 @@
     def __init__(self, *args, **kwargs):
+        ast.Record.__init__(self, *args, **kwargs)

Please avoid args/kwargs

@@ +89,3 @@
 
+    def _walk(self, callback, chain):
+        super(GLibObject, self)._walk(callback, chain)

_walk should be made public, you must not override private methods.

@@ +127,3 @@
         self.doc = None
 
+    def _walk(self, callback, chain):

Ditto see above

@@ +148,3 @@
 
+    def _walk(self, callback, chain):
+        super(GLibInterface, self)._walk(callback, chain)

Ditto see above.

::: giscanner/glibtransformer.py
@@ +61,1 @@
 class GLibTransformer(object):

This should be renamed to reflect what it does, parsing the output from dumper program

@@ +138,3 @@
+        """Load the library (or executable), returning an XML
+blob containing data gleaned from GObject's primitive introspection."""
+        from xml.etree.cElementTree import parse

Any reason not to import this at the beginning of the file?

::: giscanner/primarytransformer.py
@@ +35,3 @@
+from .utils import to_underscores, to_underscores_noprefix
+
+# -*- Mode: Python -*-

This is not the Primary, it's actually next-to-last-transformer

Needs a better name, not sure what though.

I still need to review this and the files after it, but I'm submitting it so you can start fixing it up.

(In reply to comment #29)
> Review of attachment 168888 [details] [review]:
> 
> This is unfortunate, if we're really going to reduce the types that can be used
> we
> should use the canonical non-GLib types everwhere, eg. long instead of glong,
> uint8 instead of guint8.
> Eg, seems like upstream GLib based libraries are moving from using gchar to
> char etc.
> 
> TL;DR patch

GLib/GObject/Gio aren't, others may be.  Personally I'm not often consistent (I always use char * instead of gchar *, but do use guint since it's shorter).  

However the main argument for going for the GLib types is that we need to make clear we don't support arbitrary C and POSIX stuff (FILE *, time_t), and we want to encourage library authors to stick to the GLib framework.

(In reply to comment #31)
> Review of attachment 168891 [details] [review]:
> 
> Can't be squashed with the patch that removes them?

It'd be a lot of work because then I'd have to update all of GIMarshallingTests through the .gir changes...not really worth it when I discovered that GIMarshallingTests is totally broken.

(In reply to comment #29)
> Review of attachment 168888 [details] [review]:
> 
> This is unfortunate, if we're really going to reduce the types that can be used
> we

One thing I forgot to mention here is that C authors are still perfectly free to use "char" or "gchar" as they wish; it's just that in the .gir and .typelib format, we don't have both of them.

(In reply to comment #47)
> (In reply to comment #29)
> > Review of attachment 168888 [details] [review] [details]:
> > 
> > This is unfortunate, if we're really going to reduce the types that can be used
> > we
> 
> One thing I forgot to mention here is that C authors are still perfectly free
> to use "char" or "gchar" as they wish; it's just that in the .gir and .typelib
> format, we don't have both of them.

Oh, I'm not particularly worried about the internals in the scanner and typelib, the primary concern was what should be recommended be used in the annotations, which is our public type api. I guess it's not such a big deal after all, since gchar/uint etc will not often be used.

(In reply to comment #48)
> 
> Oh, I'm not particularly worried about the internals in the scanner and
> typelib, the primary concern was what should be recommended be used in the
> annotations, which is our public type api. I guess it's not such a big deal
> after all, since gchar/uint etc will not often be used.

Oh, right; yes, we support both in annotation parsing too.

(In reply to comment #38)
> Review of attachment 168898 [details] [review]:
> 
> ::: giscanner/primarytransformer.py
> @@ +47,2 @@
>      def transform(self):
> +        # Dirty hack for now...maybe eventually we'll support the "typedef
> GSList FooSet"
> 
> So this is the typedef breakage, how much work is it to support it.

There's two typedef changes; one we don't any longer take structures with a leading _ in front, but missing a corresponding typedef.

so struct _Foo {
  int x;
  double y;
};

is no longer taken.  But struct Foo { ... } should be.

Stuff I elided was fixed.

(In reply to comment #44)
> Review of attachment 168890 [details] [review]:
> 
> ::: gir/DBus-1.0.gir
> @@ +1,2 @@
>  <?xml version="1.0"?>
> +<repository version="1.2"
> 
> All gir changes look good, but should probably be moved to separate commit

Hmm, that would break my attempts to keep "make check" working at each step.

> 
> ::: girepository/girmodule.c
> @@ +310,3 @@
>    header = (Header *)data;
>    memcpy (header, G_IR_MAGIC, 16);
> +  header->major_version = 4;
> 
> Do we document the version changes somewhere?

We don't currently; in most cases though it should be identifiable from a single commit.  In this case it's adding static methods to interfaces.

> @@ +70,3 @@
> +
> +    def __cmp__(self, other):
> +    resolved = property(lambda self: (self.target_fundamental or
> 
> Perhaps do an isinstance() to make sure we're not trying to compare against
> something different
> 
> @@ +86,3 @@
> +        any."""
> +        if isinstance(typeval, (list, tuple)):
> +            for val in typeval:
> 
> return any(self.is_equiv(val) for val in typeval)

Hmm, this doesn't support passing in a single item.  I wanted to emulate isinstance() here, so you can say:

foo.is_equiv(ast.TYPE_STRING)
or 
foo.is_equiv([ast.TYPE_STRING, ast.TYPE_FILENAME])

> @@ +272,3 @@
> +        self.version = version
> +        if identifier_prefixes:
> +            self.identifier_prefixes = identifier_prefixes
> 
> identifier_prefixes should never be [], it simplifies other code

Unfortunately, we have to support it for xlib.  

> @@ +304,3 @@
> +        """Return True if this namespace should contain the given C
> +identifier string."""
> +        if not self.identifier_prefixes:
> 
> for instance, this whole method can be written as:
> 
> return any(ident.startswith(prefix) for prefix in self.identifier_prefixes)

Not sure it's really a big deal (it's 4 simple lines to 1 line), but okay.

> @@ +398,3 @@
> +
> +    def __hash__(self):
> +        return hash((self.name, self.version))
> 
> hash(str(self)) ?

Hmm...okay.

> 
> @@ +468,1 @@
> +    def _walk(self, callback, chain):
> 
> Unused, kill?

Subclasses are expected to implement it.

> 
> @@ +529,3 @@
> +    def __init__(self, array_type, element_type, **kwargs):
> +        Type.__init__(self, target_fundamental='<array>',
> +                      **kwargs)
> 
> Please do not use **kwargs, it's too hard to use properly to avoid adding bugs
> here.
> To use properly you need to validate that there are no unknown parameters sent
> in.

I'm not sure I understand; if we have unknown parameters, the Type constructor would throw an error.

> @@ +127,3 @@
> +        self._codegen_start()
> +
> +        for node in self.namespace.itervalues():
> 
> I think __iter__ in namespace should just return the nodes, and this should
> thus be
> for node in self.namespace:

Hmmm, the whole Namespace though is designed as a mapping.  

> 
> ::: giscanner/finaltransformer.py
> @@ +21,3 @@
> +from . import glibast
> +
> +class FinalTransformer(object):
> 
> FinalTransformer is not a great name, we should rename it to something which
> makes it easier to understand.
> 
> Seems to warn and mark things as introspected, so IntrospectableTransformer?

I called it "IntrospectablePass".

> 
> ::: giscanner/girparser.py
> @@ +103,3 @@
> +
> +    def _find_children(self, node, name):
> +        result = []
> 
> maybe return [child.tag in node.children if tag.name]

Ok.

> Should probably be moved to Node and be called get_children_by_name()

These aren't Nodes, they're elementtree xml objects.

> 
> @@ +205,3 @@
> +        parent = node.attrib.get('parent')
> +        if parent:
> +            parent_type = self._namespace.type_from_name(parent)
> 
> make type_from_name(None) -> None to simplify some code here, right?

Hmmm, seems too one-off.  I'd actually like to figure out how to make the parsing more rigorous in general.  I know some projects generate code from an XML schema.  Not sure.

> ::: giscanner/glibast.py
> @@ +24,2 @@
>      def __init__(self, *args, **kwargs):
> +        ast.Record.__init__(self, *args, **kwargs)
> 
> Please avoid args/kwargs

As above though, we'll throw an error since Record actively checks, right?

> 
> @@ +89,3 @@
> 
> +    def _walk(self, callback, chain):
> +        super(GLibObject, self)._walk(callback, chain)
> 
> _walk should be made public, you must not override private methods.

I'm using "_" as "protected".  I thought "__" was actually private.

> ::: giscanner/glibtransformer.py
> @@ +61,1 @@
>  class GLibTransformer(object):
> 
> This should be renamed to reflect what it does, parsing the output from dumper
> program

Okay, I chose "GDumpParser".

> ::: giscanner/primarytransformer.py
> @@ +35,3 @@
> +from .utils import to_underscores, to_underscores_noprefix
> +
> +# -*- Mode: Python -*-
> 
> This is not the Primary, it's actually next-to-last-transformer

I chose "MainTransformer".

wip/transformer is rebased again, I won't reattach here to avoid spam.

Review of attachment 168884 [details] [review]:

Sure

Review of attachment 168888 [details] [review]:

Looks okay

::: giscanner/glibtransformer.py
@@ +1161,3 @@
         if isinstance(node, TypeContainer):
             parent = stack[-1]
+            if node.type.name in (TYPE_LONG_LONG, TYPE_LONG_DOUBLE):

Try to avoid tuples for lists, tuples is the python equivalent of a struct/record while a list is an array.

Review of attachment 168894 [details] [review]:

Sure

Review of attachment 168890 [details] [review]:

Looks quite good, a few general thoughts.

Would be great if types had a reference to their namespace, that way we could move a lot
of the convenience methods to the type/node/ast classes.
  type.to_c_type_name() -> "GtkFooBar"
  type.to_c_method_prefix() -> "gtk_foo_bar"

Or, to maintain the size of the ast classes, perhaps create a new class called TypeResolver which
does all of the type resolving logic in one place.

There's no protected in python, only public/private. xxx is public, _xxx is private, __ is not used
outside of special methods provided by python such as __init__/__repr__/etc.
The main point in marking methods private is that you know that nobody outside of the class references
them, eg code such as foo._bar is always invalid unless foo is "self" or "cls".

::: giscanner/primarytransformer.py
@@ +90,3 @@
+    # Private
+
+    def _is_gi_subclass(self, typeval, supercls_type):

Could you try to order the methods in the order they are called here?

@@ +93,3 @@
+        cls = self._transformer.lookup_typenode(typeval)
+        assert cls, str(typeval)
+        supercls = self._transformer.lookup_typenode(supercls_type)

supercls is usually "base" in python

@@ +120,3 @@
+        block = self._blocks.get(node.symbol)
+        self._apply_annotations_callable(node, chain, block)
+        if block:

invert if to reduce annotation.

@@ +122,3 @@
+        if block:
+            rename_to = block.get(TAG_RENAME_TO)
+            if rename_to:

Ditto

@@ +320,3 @@
+        if basic:
+            return basic
+        if typeval.target_giname:

Invert if

::: giscanner/scannermain.py
@@ +56,3 @@
                       action="append", dest="includes", default=[],
                       help="include types for other gidls")
+    parser.add_option('', "--test-codegen",

These options must be added to the the manual page.
Yes, we will probably generate the man page in the future, but that future is not here yet.

--generate-typelib-tests is probably a better name.
The name of the output files should at least be documented in the manual.

@@ +59,3 @@
+                      action="store", dest="test_codegen", default=None,
+                      help="Generate test code for given namespace,output.h,output.c")
+    parser.add_option('', "--passthrough-gir",

What's the point of this option, apart from being useful during testing?

@@ +93,3 @@
                       action="store", dest="namespace_version",
                       help="version of namespace for this unit")
+    parser.add_option("", "--identifier-prefix",

Remove from man page

@@ -102,3 @@
                       action="store_true", dest="verbose",
                       help="be verbose")
-    parser.add_option("", "--noclosure",

Remove these 2 from the man page

@@ +342,3 @@
     if options.output:
+        tempdir = os.path.dirname(options.output) or os.getcwd()
+        (tempfd, main_temppath) = tempfile.mkstemp(suffix='.gir', dir=tempdir)

gir = tempfile.NamedTemporaryFile(...)
gir.write()
gir.close()

...
os.rename(gir.name, ...)

@@ +347,3 @@
+        f.close()
+        if options.reparse_validate_gir:
+            (tempfd, tempgir_path) = tempfile.mkstemp(suffix='.gir', dir=tempdir)

See above

::: giscanner/testcodegen.py
@@ +1,1 @@
+# -*- Mode: Python -*-

I think this part needs more work, and while it's not used yet can you wait with committing this until it's used?
Yes, the scannermain changes would have to be removed as well.

@@ +23,3 @@
+from .codegen import CCodeGenerator
+
+INTROSPECTABLE_BASIC = filter(lambda x: x not in (

The filter/lambda combination is horrible in python.

Besides, that variable should probably go into the ast module

::: giscanner/transformer.py
@@ +274,3 @@
+        return cmp(x[2], y[2])
+
+    def split_ctype_namespaces(self, ident):

Private?

@@ +753,3 @@
         return callback
 
+    def create_type_from_user_string(self, typestr):

Private etc..

::: misc/pep8.py
@@ +184,3 @@
     """
     length = len(physical_line.rstrip())
+    if length > 99:

This should probably be in a separate changeset

(In reply to comment #54)
> Review of attachment 168888 [details] [review]:
> 
> Looks okay
> 
> ::: giscanner/glibtransformer.py
> @@ +1161,3 @@
>          if isinstance(node, TypeContainer):
>              parent = stack[-1]
> +            if node.type.name in (TYPE_LONG_LONG, TYPE_LONG_DOUBLE):
> 
> Try to avoid tuples for lists, tuples is the python equivalent of a
> struct/record while a list is an array.
.  In 2.7 there will be a literal for sets
We discussed this, I like tuples because they're read-only(In reply to comment #56)
> Review of attachment 168890 [details] [review]:
> 
> Looks quite good, a few general thoughts.
> 
> Would be great if types had a reference to their namespace, that way we could
> move a lot

I'm not really up for more serious refactoring; so deferring this one.

> of the convenience methods to the type/node/ast classes.
>   type.to_c_type_name() -> "GtkFooBar"
>   type.to_c_method_prefix() -> "gtk_foo_bar"
> 
> Or, to maintain the size of the ast classes, perhaps create a new class called
> TypeResolver which
> does all of the type resolving logic in one place.

It would be good, yes.

> ::: giscanner/primarytransformer.py
> @@ +90,3 @@
> +    # Private
> +
> +    def _is_gi_subclass(self, typeval, supercls_type):
> 
> Could you try to order the methods in the order they are called here?

It was pretty much just this random utility function I put at the top, but I moved it next to its call now.

> @@ +93,3 @@
> +        cls = self._transformer.lookup_typenode(typeval)
> +        assert cls, str(typeval)
> +        supercls = self._transformer.lookup_typenode(supercls_type)
> 
> supercls is usually "base" in python

But this is talking about GObject classes.

> ::: giscanner/scannermain.py
> @@ +56,3 @@
>                        action="append", dest="includes", default=[],
>                        help="include types for other gidls")
> +    parser.add_option('', "--test-codegen",
> 
> These options must be added to the the manual page.
> Yes, we will probably generate the man page in the future, but that future is
> not here yet.

I think of this one as "private" for now, at least I only want to support it for gjs and pygobject.  

I did however clean up more of the man page.

> --generate-typelib-tests is probably a better name.

I renamed it.

> What's the point of this option, apart from being useful during testing?

This is a hidden option to assert basically that people update GIRParser when GIRWriter is updated.  I could move it to an environment variable?
> 
> @@ +342,3 @@
>      if options.output:
> +        tempdir = os.path.dirname(options.output) or os.getcwd()
> +        (tempfd, main_temppath) = tempfile.mkstemp(suffix='.gir', dir=tempdir)
> 
> gir = tempfile.NamedTemporaryFile(...)
> gir.write()
> gir.close()
> 
> ...
> os.rename(gir.name, ...)

Ok, I switched.
> ::: giscanner/testcodegen.py
> @@ +1,1 @@
> +# -*- Mode: Python -*-
> 
> I think this part needs more work, and while it's not used yet can you wait
> with committing this until it's used?
> Yes, the scannermain changes would have to be removed as well.

It's a useful test case as is just to exercise the scanner.  

> 
> @@ +23,3 @@
> +from .codegen import CCodeGenerator
> +
> +INTROSPECTABLE_BASIC = filter(lambda x: x not in (
> 
> The filter/lambda combination is horrible in python.
> 
> Besides, that variable should probably go into the ast module

Ok, I moved it.

> ::: giscanner/transformer.py
> @@ +274,3 @@
> +        return cmp(x[2], y[2])
> +
> +    def split_ctype_namespaces(self, ident):
> 
> Private?
> 
> @@ +753,3 @@
>          return callback
> 
> +    def create_type_from_user_string(self, typestr):
> 
> Private etc..

Hmmm...I could, but at least the second one is used in maintransformer.py.  We could move more things around but as above, this code really needs to land and not accumulate more things.

> 
> ::: misc/pep8.py
> @@ +184,3 @@
>      """
>      length = len(physical_line.rstrip())
> +    if length > 99:
> 
> This should probably be in a separate changeset

Ok, done.

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