Torsten Schoenfeld <kaffeetisch gmx de> writes:
On 25.11.2010 00:43, Florian Ragwitz wrote:This set of patches changes the details of how Glib attaches pointers to SVs. The intention is to be more robust and improve interoperability with other extensions also using the PERL_MAGIC_ext mechanism.Looks great in general, thanks. This is good to have not only because it fixes the interoperability problem, but also because it encapsulates this tricky magic business in a nice bit of API. Specific comments: * It looks like gperl_find_mg is missing a couple of braces; I think as it is it will always return the first magic struct found.
Oh, bummer. I guess I added that assertion relatively late, and obviously carelessly.
* SvTYPE is not null-safe, so gperl_find_mg and gperl_remove_mg should guard against this.
I disagree. See below.
* You removed the "!sv || !SvROK (sv)" checks in a couple of places before calling SvRV (sv). Since SvRV isn't null-safe either, I think these need to stay.
Point taken there. However, I think adding those checks again is the right thing to do. Again, see below.
* The new API needs some short POD paragraphs, maybe similar in structure to what gperl_set_isa and gperl_prepend_isa have in GType.xs.
See the commit message. I don't consider the functions I added to be useful as a public interface. If a module wants to attach random pointers to arbitrary SVs and doesn't want to actually implement all of the hairy magic business, I believe it should just be using XS::Object::Magic. The reason for me making the symbols public is because they're needed in a couple of places other than GObject.xs where Glib and Gtk weren't reusing the infrastructure of GObject.xs for various reasons. Short of making those places do their job in a different or leaving them as buggy as they were, it seemed to be a reasonable thing to do to share the implementation. This is the reason the functions I added are rather lax in their argument checking. They're supposed to be called from a site already checking those things, such as gperl_get_object or gperl_get_object_check.
* I worried a while about the thread-safety implications of having one global static MGVTBL.
Don't be. Those two things are really completely unrelated. The MGVTBL is used to identify an otherwise opaque MAGIC pointer as being our own magic.
But after some pondering I now think that this exactly what we want: two scalars in two different threads representing the same object need to be associated with the same magic pointer so that they represent the same underlying GObject.
I agree. However, if we ever decided to want something different, having MGVTBLs for every thread would be the wrong thing to do. Instead, we'd toggle the MGf_DUP flag of the MAGIC and give our vtbl and svt_dup entry that would, upon thread creation, be called with the magic pointer and would be free to also create a new either clone the GObject on the C level and attaching it to the new MAGIC of the thread under construction, have the cloned SV reference the same GObject as the SV it's being cloned from, or do whatever. In addition, an svt_dup callback can also eliminate the need for tracking all perl gobjects as well as the associated global lock. However, this series of patches isn't going there just yet.
Attachment:
pgpVMKjkLp28g.pgp
Description: PGP signature