GNOME Bugzilla – Bug 625672
Add new API g_typelib_require_private()
Last modified: 2015-02-07 16:56:25 UTC
This API allows loading a typelib from a known location which is not in the search path. Primarily intended for applications that use private typelibs.
Created attachment 166847 [details] [review] Add new API g_typelib_require_private() This is equivalent to g_typelib_require() but intended for use with private typelibs, which get loaded from the provided directory.
Created attachment 166848 [details] [review] Use the new g_irepository_require_private API
N.B.: the second patch is for gedit.
Review of attachment 166847 [details] [review]: A few minor comments, otherwise looks good to commit. ::: girepository/girepository.c @@ +1166,3 @@ } +check_typelib_internal_info (GTypelib *typelib, +static gboolean "check_typelib_header" might be a better name. @@ +1183,3 @@ + const gchar *path, +check_typelib_internal_info (GTypelib *typelib, +static gboolean Should quote the filename formatter like: '%s', as we're doing for namespace. @@ +1338,3 @@ + g_free (tmp_version); + out: + ret = typelib; Missing (allow-none) @@ +1345,3 @@ + g_free (tmp_version); + out: + ret = typelib; "requires a valid version number" @@ +1387,3 @@ + namespace, version, version_conflict); + "Requiring namespace '%s' version '%s', but '%s' is already loaded", + G_IREPOSITORY_ERROR_NAMESPACE_VERSION_CONFLICT, Hm, there will be at least 3 areas of code which format %s-%s.typelib with namespace and version. May be worth refactoring into format_typelib_filename (namespace, version)
Created attachment 167040 [details] [review] Add new API g_typelib_require_private() This is equivalent to g_typelib_require() but intended for use with private typelibs, which get loaded from the provided directory. I didn't create a format_typelib_filename() function because all typelib filename generation are actually different: - %s.typelib - %s-%s.typelib - %s/%s-%s.typelib
Review of attachment 167040 [details] [review]: ::: girepository/girepository.c @@ +1177,3 @@ + const gchar *path, +check_typelib_header (GTypelib *typelib, +static gboolean Only fetch the version when you use it @@ +1192,3 @@ + g_set_error (error, G_IREPOSITORY_ERROR, + G_IREPOSITORY_ERROR_NAMESPACE_MISMATCH, + "Typelib file %s for namespace '%s' contains " Missing quotes for file @@ +1319,2 @@ + goto out; + if (!check_typelib_header (typelib, path, namespace, version, error)) Can't see all the code above, but typelib might be leaked here @@ +1334,3 @@ + g_free (tmp_version); + out: + ret = typelib; Add this to gi-sections.txt @@ +1344,3 @@ + g_free (tmp_version); + out: + ret = typelib; It should probably make the version optional, eg accept NULL for consistency with the other APIs @@ +1414,3 @@ + typelib = g_typelib_new_from_mapped_file (mfile, &temp_error); + GError *temp_error = NULL; + { typelib is leaked here.
Review tool failed, let's clarify: New API should be added to gi-sections.txt, typelib is leaed after the check_typelib_header calls.
Created attachment 167071 [details] [review] Add new API g_typelib_require_private() This is equivalent to g_typelib_require() but intended for use with private typelibs, which get loaded from the provided directory. This approach reuses most of the code of g_irepository_require(), and allows version=NULL (the previous patch didn't).
Created attachment 167072 [details] [review] Do not leak typelibs with wrong header info. Previously the typelibs that were loaded but whose header information weren't right were just leaked.
Review of attachment 167072 [details] [review]: Looks fine.
Review of attachment 167071 [details] [review]: Looks fine, thanks!
Attachment 167071 [details] pushed as e59232e - Add new API g_typelib_require_private() Attachment 167072 [details] pushed as 3f4a605 - Do not leak typelibs with wrong header info.
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]