GNOME Bugzilla – Bug 689482
gerror: Allow calling .matches() etc. on unpaired error enums
Last modified: 2012-12-02 21:47:32 UTC
When we introduced the nicer GError handling, we broke the case of using "unpaired" errors, because method invocation assumed that we had a paired instance. Fix this by checking both the boxed case and the new GLib_Error class cases, and extracting the target GError. Unfortunately does not come with a test, because regress.[ch] in g-i is configured with fatal warnings, and unpaired errors are fatal. But this fixes using G_SPAWN_EXIT_ERROR. See: https://bugzilla.gnome.org/show_bug.cgi?id=689452
Created attachment 230442 [details] [review] gerror: Allow calling .matches() etc. on unpaired error enums
Review of attachment 230442 [details] [review]: One of the reason I originally wrote the typechecking patch was to make private accessors safe in paths that cannot call JS_BeginRequest, so it should not throw on instances. Also, gjs_typecheck_gerror() is called in other places in arg.c, and they suffer from the same problem. Personally, I'd make gjs_typecheck_gerror() account for boxed too, and then write gerror_from_error in a way that it never throws. Otherwise, we could rely on Boxed more, and layout the GError private struct with a Boxed as the first member, so the boxed accessor would work for both.
Created attachment 230449 [details] [review] tests: Start using WarnLib from g-i, add a test for unpaired error
(In reply to comment #2) > Review of attachment 230442 [details] [review]: > > One of the reason I originally wrote the typechecking patch was to make private > accessors safe in paths that cannot call JS_BeginRequest, so it should not > throw on instances. But it appears currently that *every* call to gjs_typecheck_gerror passes JS_TRUE for "throws" - so the above patch isn't regressing anything, unless I misunderstand. > Personally, I'd make gjs_typecheck_gerror() account for boxed too, and then > write gerror_from_error in a way that it never throws. > Otherwise, we could rely on Boxed more, and layout the GError private struct > with a Boxed as the first member, so the boxed accessor would work for both. I'm not opposed to either of those, but since you know this code better, would you mind taking a shot at that? I wrote infrastructure for the test case, you should be able to roll the above patch into yours.
Created attachment 230457 [details] [review] GError: pretend that GBoxed wrapping GError is a JS GError gjs_error_from_gerror() will return a JS object with a plain Boxed JS class in case it doesn't find the errordomain metadata. Later, gjs_typecheck_gerror() would choke on those, so they would be effectively useless. Instead, recognize GError wrappers with a boxed JS class and delegate typecheck and structure access. This is what I had in mind. Not tested because I don't have a warnlib.c from gobject-introspection...
(In reply to comment #5) > This is what I had in mind. > Not tested because I don't have a warnlib.c from gobject-introspection... It's in the Depends: of this bug. Review that first? =)
Ooops, I hadn't noticed that :) Works like a charm anyway.
Review of attachment 230457 [details] [review]: This looks pretty simple and correct. But please squash it with my patch to add the test before committing.
Attachment 230457 [details] pushed as aa1db67 - GError: pretend that GBoxed wrapping GError is a JS GError