GRefPtr should always use g_variant_ref_sink to deal with GVariant floating references. In case of full references, g_variant_ref_sink calls g_variant_ref, so it's safe to use it always.
Created attachment 221582 [details] Patch
(In reply to comment #0) > GRefPtr should always use g_variant_ref_sink to deal with GVariant floating references. In case of full references, g_variant_ref_sink calls g_variant_ref, so it's safe to use it always. Being this the case, if I am understanding GRefPtr correctly then we should never use adoptGRef() for GVariants, because it does *not* sink the floating reference or add a reference. If that's the case it would be good to add a specialization of the adoptGRef() template to make code that uses it work properly with GVariants. I see two possible ways: a. Specialize adoptGRef<GVariant>() to *not* pass GRefPtrAdopt as second argument. This would make adoptGRef() sink the floating reference: template<> GRefPtr<GVariant> adoptGRef(GVariant* p) { return GRefPtr<GVariant>(p); } b. Make the compiler stop with error when using adoptGRef<GVariant>(). For example using a “static_assert” depending on the type used for GRefPtr. This would prevent using adoptGRef() for GVariants. For example the following would cause compiler errors when instantiating adoptGRef() for GVariants: // In general, adoptGRef() can be used for any type. template <typename T> class GRefPtr { // ... static const bool canUseAdoptGRef = true; }; // But not for GVariant, so specialize the value of the flag. template <> class GRefPtr<GVariant> { static const bool canUseAdoptGRef = false; }; template <typename T> GRefPtr<T> adoptGRef(T* p) { static_assert(GRefPtr<T>::canUseAdoptGRef, "Can't use adoptGRef() for this type"); return GRefPtr<T>(p, GRefPtrAdopt); } Probably option (a) is the less intrusive, and it would avoid having to go through the existing code chasing the existing uses of adoptGRef(). WDYT?
You might want to adopt a full reference. We don't want to use adoptGRef with GVariants having a floating reference
(In reply to comment #3) > You might want to adopt a full reference. We don't want to use adoptGRef with GVariants having a floating reference Then, could you please add something like the solution outlined in (b) to make it a compiler error to use adoptGRef() on a GVariant? That would prevent all of us of making the mistake.
(In reply to comment #4) > (In reply to comment #3) > > You might want to adopt a full reference. We don't want to use adoptGRef with GVariants having a floating reference > > Then, could you please add something like the solution outlined in (b) > to make it a compiler error to use adoptGRef() on a GVariant? That would > prevent all of us of making the mistake. Using adoptGRef with a GVariant is not a mistake, if the GVariant has a full reference and you want to adopt it. That's what happens with some methods like g_dbus_proxy_call_sync that return a full reference that you want to adopt. We might check if the GVariant has a floating reference and assert (or warning) in that case, but I don't think it's worth it, TBH. You are supposed to know how it works, like with GtkWidget, even when not using smart pointers. If it helps I can document in the wiki how to use initially unowned references with our smart pointers.
Committed r162306: <http://trac.webkit.org/changeset/162306>
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > You might want to adopt a full reference. We don't want to use adoptGRef with GVariants having a floating reference > > > > Then, could you please add something like the solution outlined in (b) > > to make it a compiler error to use adoptGRef() on a GVariant? That would > > prevent all of us of making the mistake. > > Using adoptGRef with a GVariant is not a mistake, if the GVariant has a full reference and you want to adopt it. That's what happens with some methods like g_dbus_proxy_call_sync that return a full reference that you want to adopt. We might check if the GVariant has a floating reference and assert (or warning) in that case, but I don't think it's worth it, TBH. You are supposed to know how it works, like with GtkWidget, even when not using smart pointers. If it helps I can document in the wiki how to use initially unowned references with our smart pointers. Ah, understood. I am writing a wiki page explaining how to use GRefPtr so it is easier for people starting to hack in WebKitGTK+ to use it properly.
(In reply to comment #7) > (In reply to comment #5) > > (In reply to comment #4) > > > (In reply to comment #3) > > > > You might want to adopt a full reference. We don't want to use adoptGRef with GVariants having a floating reference > > > > > > Then, could you please add something like the solution outlined in (b) > > > to make it a compiler error to use adoptGRef() on a GVariant? That would > > > prevent all of us of making the mistake. > > > > Using adoptGRef with a GVariant is not a mistake, if the GVariant has a full reference and you want to adopt it. That's what happens with some methods like g_dbus_proxy_call_sync that return a full reference that you want to adopt. We might check if the GVariant has a floating reference and assert (or warning) in that case, but I don't think it's worth it, TBH. You are supposed to know how it works, like with GtkWidget, even when not using smart pointers. If it helps I can document in the wiki how to use initially unowned references with our smart pointers. > > Ah, understood. I am writing a wiki page explaining how to use GRefPtr > so it is easier for people starting to hack in WebKitGTK+ to use it > properly. Done: https://trac.webkit.org/wiki/GRefPtr