GNOME Bugzilla – Bug 709700
Re-organize and cleanup argument cachers into files based on type
Last modified: 2014-02-06 03:31:41 UTC
Part I: The idea is to move creation, marshaling, cleanup, and destruction specific to an argument cache type into a single file. We currently have many large files (pygi-cache, pygi-cleanup, pygi-marshal-from-py, and pygi-marshal-to-py) with a types functions distributed through them and mixed in with all the other types functions of similar nature. This makes maintenance difficult and hard for contributors to understand the architecture as well as know of potential side effects when working on any one part of a types functionality. With some re-organization, we can start to move from a pseudo object oriented cache system into something a bit more concise. ArgCache is a class which contains a set of standard virtual functions for construction, cleanup and marshaling, all of which should live in the same file based on the sub-type being implemented. A preliminary proposal of files is as follows: pygi-arg-basictypes.c (ints, strings, etc) pygi-arg-arrays.c (C, GArray, GPtrArray) pygi-arg-lists.c (GList, GSList) pygi-arg-structs.c (Struct, Boxed, GValue, GVariant, GObject?) pygi-arg-hashtable.c (GHashTable) All functionality contained within pygi-cleanup, pygi-marshal-from-py and pygi-marshal-to-py would be split up into these files. Some of the type specific functionality in pygi-cache for ArgCache instance creation and setup should also be moved into the file based on type. pygi-cache should stay around for generic setup of cache types shared by all types as well as dispatching the creation of the different types (and creation of PyGICallableCache which manages all individual arg caches). Likewise, it would be good to split up unittests which have grown rather large into files closer to the arg type files proposed above (breaking down test_gi, test_everything, etc...). Part II (this could actually go before or after Part I): There are a couple of cases where the individual ArgCache types have intimate knowledge of the containing structure (PyGICallableCache). This consists of types which have a dependency on a sibling ArgCaches (C array and its length or a callback with its user_data or destroy function). It would be worthwhile to clean this up so sibling cache management is part of PyGICallableCache (or its invoke) which passes the dependent sibling to the primary ArgCache marshaling APIs directly, cleaning up the breakage of encapsulation. https://git.gnome.org/browse/pygobject/tree/gi/pygi-marshal-from-py.c?id=3.10.0#n900 Similarly there are a couple of cases where access to PyGIInvokeState.args_data needed from within individual arg marshalers. This is for storing cleanup data when the argument is inout because the arguments pointer (GIArgument.v_pointer) is clobbered when invoke is called for marshaling back to python. This also occurs for closures where cleanup data is different from the closure passed to invoke. See: https://git.gnome.org/browse/pygobject/tree/gi/pygi-marshal-from-py.c?id=3.10.0#n926 https://git.gnome.org/browse/pygobject/tree/gi/pygi-marshal-from-py.c?id=3.10.0#n1330 This basically boils down to: * Don't pass PyGICallableCache or PyGIInvokeState pointers to marshaler vfuncs because it breaks encapsulation and requires co-dependent interface circularity between parent and child in the caching hierarchy. * Instead pass an out pointer for cleanup data and an auxiliary ArgCache and GIArgument for dependent sibling marshaling managed by the parent PyGICallableCache structure. The API change for marshalers would then move from this: gboolean from_py_marshaler (PyGIInvokeState *state, PyGICallableCache *callable_cache, PyGIArgCache *arg_cache, PyObject *py_arg, GIArgument *arg); To something like this: gboolean from_py_marshaler (PyGIArgCache *arg_cache, /* self */ PyObject *py_arg, /* in */ GIArgument *arg, /* out */ GIArgument *arg_aux, /* out */ gpointer *cleanup_data /* out */);
Marking bug 693402 as needing prior completion. The concept of "cleanup_data" passed as an out arg to from_py marshalers is already implemented in a patch there to help memory management.
Created attachment 257072 [details] [review] cache refactoring: Separate ArgCache creation and setup Move PyGIArgCache and PyGIInterfaceCache generic setup into standalone functions: pygi_arg_base_setup and pygi_arg_interface_setup respectively. Shift argument order and move arguments which will eventually be removed from the signature into the tail with comment. Isolate special casing for GI_INFO_TYPE_CALLBACK ArgCache creation to a single location in _arg_cache_new_for_interface.
Created attachment 257073 [details] [review] cache refactoring: Move PyGIHashCache and related marshaling into new file Massive re-org of hash table arg cache and its marshaling by moving all related code fragments into an isolated file where most functions are made static. pygi-arg-hashtable.h exposes a single function: pygi_arg_hash_table_new_from_info. This is all the caching system needs to produce the proper bits for handling hash table marshaling.
Created attachment 257079 [details] [review] cache refactoring: Move basic type arg setup and marshaling into new file Move all basic type arg caching and marshaling fragments into an isolated file where most functions are made static. pygi-arg-basictype.h exposes: pygi_arg_hash_table_new_from_info, _pygi_marshal_from_py_basic_type, and _pygi_marshal_to_py_basic_type which allows continued use for all marshaling code paths.
Created attachment 257083 [details] [review] cache refactoring: Break sequence cache up for array vs list Add new arg cache type specialized for arrays. This cleans up the basic sequence cache type which does not need length and size related info. Remove fixed length checks from GList and GSList from_py marshaling because these will always be -1. Notes: This is prelim work for breaking GList/GList and GArray fragments into their own files. There are a few naming consistency issues because I tried to keep this patch from touching as much stuff as possible. This can be cleaned up in a later naming consistency pass. You will notice various lines like "arg_cache->py_arg_index = py_arg_index" spreading across _arg_cache_new dispatching. These lines will become isolated to the end of the function once all dispatching uses the new model for creation/setup.
Created attachment 257088 [details] [review] cache refactoring: Move GList/GSList arg setup and marshaling into new file Move GList and GSList argument caching and marshaling fragments into an isolated file: pygi-arg-glist.c.
Created attachment 257114 [details] [review] cache refactoring: Move GError arg setup and marshaling to new file Move GError argument caching and marshaling fragments into isolated file: pygi-arg-gerror.c.
Created attachment 257122 [details] [review] cache refactoring: Move callback setup and marshaling into new file Move callback argument caching and marshaling fragments into isolated file: pygi-arg-callback.c.
Created attachment 257136 [details] [review] cache refactoring: Move GObject arg setup and marshaling into new file Move GObject argument cache setup and marshaling fragments into isolated file: pygi-arg-callback.c. Break GIInterfaceCache creation and setup into API for interface based argument cache usage.
Created attachment 257144 [details] Fix GArray, GList, GSList, and GHashTable marshaling leaks Additional cleanup, addition of G_BEGIN/END_DECLS, rebase. https://bugzilla.gnome.org/show_bug.cgi?id=693402
Created attachment 257145 [details] [review] cache refactoring: Separate ArgCache creation and setup Additional cleanup, addition of G_BEGIN/END_DECLS, rebase.
Created attachment 257146 [details] [review] cache refactoring: Move PyGIHashCache and related marshaling into new file Additional cleanup, addition of G_BEGIN/END_DECLS, rebase.
Created attachment 257147 [details] [review] cache refactoring: Move basic type arg setup and marshaling into new file Additional cleanup, addition of G_BEGIN/END_DECLS, rebase.
Created attachment 257148 [details] [review] cache refactoring: Break sequence cache up for array vs list Additional cleanup, addition of G_BEGIN/END_DECLS, rebase.
Created attachment 257149 [details] [review] cache refactoring: Move GList/GSList arg setup and marshaling into new file Additional cleanup, addition of G_BEGIN/END_DECLS, rebase.
Created attachment 257150 [details] [review] cache refactoring: Move GArray arg setup and marshaling into new file Additional cleanup, addition of G_BEGIN/END_DECLS, rebase.
Created attachment 257151 [details] [review] cache refactoring: Move GError arg setup and marshaling to new file Additional cleanup, addition of G_BEGIN/END_DECLS, rebase.
Created attachment 257152 [details] [review] cache refactoring: Move callback setup and marshaling into new file Additional cleanup, addition of G_BEGIN/END_DECLS, rebase.
Created attachment 257153 [details] [review] cache refactoring: Move GObject arg setup and marshaling into new file Additional cleanup, addition of G_BEGIN/END_DECLS, rebase.
Created attachment 257154 [details] [review] cache refactoring: Move various struct arg setup and marshaling to new file Move struct (boxed, union, gvalue, gclosure, variant, and pointer) argument cache setup and marshaling fragments into isolated file: pygi-arg-struct.c. Remove redundant and dead code related to boxed and union marshaling.
Created attachment 257155 [details] [review] cache refactoring: Move enum and flags arg setup and marshaling to new file Move enum and flags argument cache setup and marshaling fragments into isolated file: pygi-arg-enum-flags.c Final removal of pygi-marshal-from/to files.
Comment on attachment 257144 [details] Fix GArray, GList, GSList, and GHashTable marshaling leaks This was added to the wrong bug.
Created attachment 267882 [details] [review] cache refactoring: Separate ArgCache creation and setup Rebased
Created attachment 267883 [details] [review] cache refactoring: Move PyGIHashCache and related marshaling into new file Rebased and renamed to pygi-hashtable.[h|c]
Created attachment 267884 [details] [review] cache refactoring: Move basic type arg setup and marshaling into new file Rebased
Created attachment 267885 [details] [review] cache refactoring: Break sequence cache up for array vs list Rebased
Created attachment 267886 [details] [review] cache refactoring: Move GList/GSList arg setup and marshaling into new file Rebased and renamed files to pygi-list.[h|c]
Created attachment 267887 [details] [review] cache refactoring: Move GArray arg setup and marshaling into new file Rebased and renamed files to pygi-array.[h|c]
Created attachment 267889 [details] [review] cache refactoring: Move GError arg setup and marshaling to new file Rebased and renamed files to pygi-error.[h|c]
Created attachment 267890 [details] [review] cache refactoring: Move closure setup and marshaling into pygi-closure Rebased and moved code fragments into existing pygi-closure.c
Created attachment 267891 [details] [review] cache refactoring: Move GObject arg setup and marshaling into new file Rebased and renamed new files to pygi-object.[h|c]
Created attachment 267892 [details] [review] cache refactoring: Move various struct arg setup and marshaling to new file Rebased and renamed new files to pygi-struct-marshal.[h|c]
Created attachment 267893 [details] [review] cache refactoring: Move enum and flags arg setup and marshaling to new file Rebased and renamed new files to pygi-enum-marshal.[h|c]
Created attachment 267894 [details] [review] marshal refactoring: Move GIArgument from GValue code to new file Add gi/pygi-value.h and .c files with initial contents of _pygi_argument_from_g_value. Eventually this file will contain all code related to GValue marshaling from various code locations in the project.
Created attachment 267895 [details] [review] marshal refactoring: Move GValue marshaling from pytype into pygi-value Move marshaling of GValues to and from PyObjects into pygi-value.c. Make PyGTypeMarshal struct and related functions accessible via pygtype.h.
Review of attachment 267882 [details] [review]: This is mostly just refactoring, LGTM. However, I haven't actually ever looked at that code in detail, so please don't take this as "verified to work" :-) But I have quite a lot of faith in our test suite. ::: gi/pygi-cache.c @@ +71,3 @@ + GIArgInfo *arg_info, /* may be NULL for return arguments */ + GITransfer transfer, + PyGIDirection direction) It's not quite clear from the name what "arg base setup" means; could you add a comment about that and pygi_arg_interface_setup() as well?
Review of attachment 267883 [details] [review]: LGTM, except for that constructor name. Thanks! ::: gi/pygi-cache.h @@ +212,3 @@ GIInterfaceInfo *iface_info); +PyGIArgCache * _arg_cache_new (GITypeInfo *type_info, This looks inconsistant to me: All other methods for PyGIArgCache start with _pygi, why doesn't that constructor?
Review of attachment 267884 [details] [review]: With this commit I'm starting to get a "feel" for how the reorg will look like. I'm not 100% sure that organizing it by data type instead of by direction will make things much easier to understand, but this definitively trims the necessary internal API; and if one wants to change the marshalling for a particular type, everything should eventually be in just one file, so that's good. Thanks!
Review of attachment 267885 [details] [review]: LGTM.
Review of attachment 267886 [details] [review]: LGTM
Review of attachment 267887 [details] [review]: Same old, same old :-)
Review of attachment 267889 [details] [review]: LGTM.
Review of attachment 267890 [details] [review]: Already said on IRC, but for the record: I really like that the from/to python marshalling is now "hidden" behind the ArgCache API, so it's much easier to understand the structure now and avoid having to change things outside the pygi-<datatype>.c file. ::: gi/pygi-closure.h @@ +13,3 @@ * * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see <http://www.gnu.org/licenses/>. Nice catch! (There recently was some initiative to fix old copyright headers in GNOME)
Review of attachment 267891 [details] [review]: Ack.
Review of attachment 267892 [details] [review]: Ack.
Review of attachment 267893 [details] [review]: Ack.
Review of attachment 267894 [details] [review]: Ack.
Attachment 267882 [details] pushed as 983d0c2 - cache refactoring: Separate ArgCache creation and setup Attachment 267883 [details] pushed as 4a6bf3b - cache refactoring: Move PyGIHashCache and related marshaling into new file Attachment 267884 [details] pushed as c1a2a86 - cache refactoring: Move basic type arg setup and marshaling into new file Attachment 267885 [details] pushed as c48ddac - cache refactoring: Break sequence cache up for array vs list Attachment 267886 [details] pushed as 4697a37 - cache refactoring: Move GList/GSList arg setup and marshaling into new file Attachment 267887 [details] pushed as c45cafd - cache refactoring: Move GArray arg setup and marshaling into new file Attachment 267889 [details] pushed as 18d8274 - cache refactoring: Move GError arg setup and marshaling to new file Attachment 267890 [details] pushed as 2cddba8 - cache refactoring: Move closure setup and marshaling into pygi-closure Attachment 267891 [details] pushed as 4dcaa2b - cache refactoring: Move GObject arg setup and marshaling into new file Attachment 267892 [details] pushed as 1d0f120 - cache refactoring: Move various struct arg setup and marshaling to new file Attachment 267893 [details] pushed as c2d5857 - cache refactoring: Move enum and flags arg setup and marshaling to new file Attachment 267894 [details] pushed as b8120d8 - marshal refactoring: Move GIArgument from GValue code to new file Attachment 267895 [details] pushed as 204f5a1 - marshal refactoring: Move GValue marshaling from pytype into pygi-value
(In reply to comment #36) > It's not quite clear from the name what "arg base setup" means; could you add a > comment about that and pygi_arg_interface_setup() as well? (In reply to comment #37) > +PyGIArgCache * _arg_cache_new (GITypeInfo *type_info, > > This looks inconsistant to me: All other methods for PyGIArgCache start with > _pygi, why doesn't that constructor? I will commit fixes for these separately because the modifications cause a bad re-base chain.
(In reply to comment #52) > (In reply to comment #36) > > It's not quite clear from the name what "arg base setup" means; could you add a > > comment about that and pygi_arg_interface_setup() as well? > > (In reply to comment #37) > > +PyGIArgCache * _arg_cache_new (GITypeInfo *type_info, > > > > This looks inconsistant to me: All other methods for PyGIArgCache start with > > _pygi, why doesn't that constructor? > > I will commit fixes for these separately because the modifications cause a bad > re-base chain. Related commits: https://git.gnome.org/browse/pygobject/commit/?id=56ac6bd https://git.gnome.org/browse/pygobject/commit/?id=f3be4ce