GNOME Bugzilla – Bug 555960
Nested structs and unions
Last modified: 2015-02-07 16:55:09 UTC
The attached patch bundle adds support for nested structs/unions, including unnamed member structs/unions.
Created attachment 120413 [details] [review] Patch to add nested structs/union support
Comment on attachment 120413 [details] [review] Patch to add nested structs/union support This looks great, thanks for doing this. Out of curiosity, what is this needed for GdkEvent & friends? I haven't reviewed the typelib changes very careful since I'm not that familiar with it. Do we need to update the .txt specification? >+2008-10-12 Andreas Rottmann <a.rottmann@gmx.at> Can you merge the two changelog entries? It would be a bit more useful to describe why and not what you change. >=== modified file 'girepository/girparser.c' >+pop_node (ParseContext *ctx) >+{ >+ g_assert (ctx->node_stack != 0); >+ >+ GSList *top = ctx->node_stack; C99, please rearrange so it works on C89 compilers. >+ if (strcmp (element_name, "record") == 0 >+ && VALID_STRUCT_STATE (ctx->state)) >+ if (strcmp (element_name, "union") == 0 && >+ VALID_UNION_STATE(ctx->state)) I'd just remove these macros, to avoid a layer of indirection where it is not needed. >=== modified file 'giscanner/transformer.py' >+ def _create_union(self, symbol, anonymous=False): There's a fair amount of duplication between create_union/create_struct, can it be avoided without getting too messy? >=== modified file 'tests/scanner/Makefile.am' > %.typelib: %.gir $(top_builddir)/tools/g-ir-compiler$(EXEEXT) Makefile > $(top_builddir)/tools/g-ir-compiler --includedir=. --includedir=$(top_builddir)/gir $< -o $@ >- $(SCANNER) --typelib-xml $< > $<.tmp && mv $<.tmp $<.txml >+ PYTHONPATH=$(top_builddir):$$PYTHONPATH $(CHECK_DEBUG) $(SCANNER) --typelib-xml $< > $<.tmp && mv $<.tmp $<.txml Good catch. I think this fixes a couple of bugs I've run into before.
Is it correct to say the typelib changes make the compiler accept but ignore the nested structs/unions? In other words the nested components aren't in the typelib?
Created attachment 120962 [details] [review] New patch, suggestions implemented > This looks great, thanks for doing this. > Out of curiosity, what is this needed for GdkEvent & friends? > I've not yet looked at GdkEvent; I need these changes to parse and work with the GIR that's generated by scanning gtypelib.h. > I haven't reviewed the typelib changes very careful since I'm not that familiar with it. > Do we need to update the .txt specification? > > Is it correct to say the typelib changes make the compiler accept but ignore > the nested structs/unions? In other words the nested components aren't in the > typelib? I still need to look into this; as I don't use/bind any typelib that makes use of these features, I just don't know :-) (as I said, I only need the generated GIR to exhibit this stuff ATM). > ... I implemented all the suggestions from jdahlin; note the build fix is not included; I'll attach it to Bug 557147.
If you're writing a binding especially for a dynamic language, you probably want to be using the typelib in general. The current plan is for the .typelib files to be installed with the .so files (i.e. in libfoo package), and for the .gir files to go with the headers (i.e. in libfoo-devel package). Now for a static language it can make sense to have a compile step where you read the .gir files and output language-specific glue. However for JGIR I chose to use the .typelib since I think it is actually easier in the end than parsing XML. It's also more stable by nature. So I'm not sure how valuable it is going to be to have nested data only in the .gir. We should either figure out how it works in the typelib, or not support it. That decision rests on what uses it, how important those things are, and how easy is it is to work around/change.
> If you're writing a binding especially for a dynamic language, you probably > want to be using the typelib in general. The current plan is for the .typelib > files to be installed with the .so files (i.e. in libfoo package), and for the > .gir files to go with the headers (i.e. in libfoo-devel package). > I'm doing a binding for a dynamic language (Scheme, specifically Ikarus (an R6RS implementation), although only a small part of the code is implementation-specific, a port to other R6RS implementations with a decent low-level FFI to C (MzScheme pretty sure, Ypsilon probably, and Larceny maybe -- I still haven't figured out how to use the FFI in R6RS mode; there may be R6RS implementations with C FFIs I'm not aware of). I use the typelib directly, *but* I use the GIR generated from gtypelib.h to figure out how to destructure the typelib blob. > Now for a static language it can make sense to have a compile step where you > read the .gir files and output language-specific glue. However for JGIR I > chose to use the .typelib since I think it is actually easier in the end than > parsing XML. It's also more stable by nature. > Well, despite Scheme being a fairly dynamic language, my approach indeed involves a compile-like step: At macro-expansion-time (which roughly equates to compile-time), the GIR is used to generate code that defines accessors for the various structs and unions defined in gtypelib.h. Of course I could rather use the girepository library to do that parsing for me, but then I would have to bind all the functions manually, and wouldn't have a had created a nice C compound struct/union destructurer ;-). Or I could hardcode this info in some file and update that manually when the typelib format changes (I'll probably choose some variant of this when this patch doesn't go in or gtypelib.h is adapted so that when it's processed by g-ir-scanner useful information is produced without this patch). I grant that this is indeed a kind of "special use", but IMHO, there's some value in making g-ir-scanner more general and able to handle a wider subset of C, without considering the merit for that special use case.
Hmm...while the typelib format is relatively stable I'm not sure I'd want another consumer parsing it manually. It isn't really intended to be public right now. What I suggest is to manually bind the repository API (girepository.h), and once you have that and some custom bindings for a bit of GObject (signals, properties etc.) you're good to go. But going back to the big picture on this bug - yes, we may need the recursive data in the GIR, at least for bug 551738.
Created attachment 121293 [details] [review] Rebased patch, should apply cleanly against SVN r812 This still does not compile the nested entities into the typelib, and hence causes a "make check" failure. I'll tackle this next.
Formally marking the patch as needing more work.
Created attachment 122386 [details] [review] Extend girparser.c to keep stack of nodes I've now split out the girparser.c node_stack changes, as these are completely independent, and I need them for another patch (fixing 560241) which I'm working on.
Comment on attachment 122386 [details] [review] Extend girparser.c to keep stack of nodes >@@ -2465,6 +2487,41 @@ > } > > static gboolean >+state_switch_end_struct_or_union (GMarkupParseContext *context, >+ ParseContext *ctx, >+ const gchar *element_name, >+ GError **error) >... Ignore this hunk, it accidentially slipt in.
Created attachment 122614 [details] [review] Extend girparser.c to keep stack of nodes (rebased against SVN r919) (This also doesn't include the spurious hunk)
Created attachment 122616 [details] [review] Implement nested structs/unions (WIP) (against SVN r919) This is an updated version of the patch, excluding the girparser.c node-stack changes. It still doesn't include typelib support, but I thought I'd post it anyway, since someone might want to play around with it.
This patch looks good, it's just incomplete - we should have a plan for how this goes into the typelib if we're going to support it. Right now the typelib has a totally flat namespace; it's effective just a list of names, in contrast to the nesting the GIR XML allows. Now we could try to make a generic nesting system, but that would be a lot of nontrivial difficult work. One easier idea is allow the '/' character in typelib names. This is is fairly straightforward in the case of nested *named* structures. struct FooDatum { int q; union { double z; void *v; } u; }; The inner union here could go into the typelib as "Datum/u" (namespace Foo). But how to handle anonymous nested unions, like your UtilityTaggedValue? Well, one approach is to just make up a name; so we'd have "TaggedValue/<anon0>". What bindings could do is if you say value.z, try to look up "z" in the value itself, when that fails, call g_struct_info_get_anonymous () to get an array of the anonymous members, and look up in each one.
Andreas do you want this patch in for now? What do you think about the idea of making up names for the anonymous members?
(In reply to comment #15) > Andreas do you want this patch in for now? > Yeah, I'd like to get this (as incomplete as it is) in, as it would relieve me from constant rebasing (the patch is regularly broken by code changes). I'll try to get it rebased against current SVN once again. > What do you think about the idea of making up names > for the anonymous members? > I think it's a reasonable solution, when a "cleaner" one would require considerably more effort.
Agreed we should get this patch in. I want to fix this bug since it will greatly help bug 551738. I'm now generating javadoc for JGIR, and all of the class structs appearing as regular structures is *ugly*. So...let's set aside the patch in 551738, get this patch in. We can then figure out how class structs should appear in the GIR. At present, the only consumers of the .typelib would not be making use of class layout information (I would like to for JGIR eventually), and there aren't many important nested structures in most GObject APIs. Andreas did you say you had a rebased version of this patch? The one in this bug has a lot of conflicts vs trunk.
Created attachment 127829 [details] [review] Extend girparser.c to keep stack of nodes (rebased against SVN r1080)
Created attachment 127830 [details] [review] Implement nested structs/unions (WIP) (against SVN r1080)
Ok, merged both patches, with a minor tweak to remove the FIXMEs in the expected tgir.
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]