GNOME Bugzilla – Bug 406960
TypeTraits<bool> utterly broken
Last modified: 2007-08-03 23:11:55 UTC
I just discovered this little gem: template <> struct TypeTraits<bool> { typedef bool CppType; typedef gboolean* CType; typedef gboolean* CTypeNonConst; static CType to_c_type (CppType val) { return (int*)GINT_TO_POINTER(val); } static CType to_c_type (CType ptr) { return ptr; } static CppType to_cpp_type(CType ptr) { if(ptr) { //We use this for gboolean too, because it is actually an int. return GPOINTER_TO_INT(ptr); } else return CppType(); } static void release_c_type(CType /* ptr */) { } }; Bllaaaaarghh! This is completely and utterly broken. Looks like the KeyFile addition is responsible: 2006-02-20 Rob Page <page.rob@gmail.com> Wraps GKeyFile (Bug #330535) * glib/glibmm.h: Added include of keyfile.h * glib/glibmm/Makefile.am: Added keyfile.h * glib/glibmm/containerhandle_shared.h: Added a TypeTraits specialization for converting between bool and gboolean*. * glib/src/Makefile_list_of_hg.am_fragment: Added keyfile.hg to files_general_hg. * glib/src/keyfile.hg: KeyFile header * glib/src/keyfile.ccg: KeyFile implementation * tools/m4/convert_glib.m4: Added a conversion for KeyFileFlags It's used here: Glib::ArrayHandle<bool> KeyFile::get_boolean_list(const Glib::ustring& group_name, const Glib::ustring& key) const { gboolean* bool_list = 0; gsize length_of_list = 0; GError* error = 0; bool_list = g_key_file_get_boolean_list(const_cast<GKeyFile*>(gobj()), group_name.c_str(), key.c_str(), &length_of_list, &error); if(error) Glib::Error::throw_exception(error); return Glib::ArrayHandle<bool>(&bool_list, length_of_list, Glib::OWNERSHIP_DEEP); } I bet my ass this code will cause hell to break loose at runtime. The actual mistake which probably prompted the author to add the TypeTraits<bool> specialization (*aaarrghg!*) is rather subtle: return Glib::ArrayHandle<bool>(&bool_list, length_of_list, ...) ^^^ That & is one & too many. The ownership should be SHALLOW, too, but that's comparatively minor in this case as DEEP ownership is meaningless for arrays of plain old values. Strangely enough, KeyFile::get_integer_list() does it right (except for the ownership), even though the code is almost identical. I dearly hope that we're not using *_Handle<bool> anywhere else throughout the library stack, or they might be ABI fun to deal with.
Do you have a patch?
Created attachment 82761 [details] [review] The fix plus some cleanup Here's the patch. It turns out that a TypeTraits<bool> specialization is in fact necessary, though of course it has to look quite different: template <> struct TypeTraits<bool> { typedef bool CppType; typedef gboolean CType; typedef gboolean CTypeNonConst; static CType to_c_type (CppType item) { return static_cast<CType>(item); } static CType to_c_type (CType item) { return item; } static CppType to_cpp_type (CType item) { return (item != 0); } static void release_c_type (CType) {} }; Essentially a bunch of no-ops, and no pointers appearing out of nowhere anymore. The patch also includes some moderate cleanup of keyfile.ccg. Unfortunately, I noticed another API bug: void set_boolean_list(const Glib::ustring& group_name, const Glib::ustring& key, Glib::ArrayHandle<bool>& list); void set_integer_list(const Glib::ustring& group_name, const Glib::ustring& key, Glib::ArrayHandle<int>& list); The 'list' parameters should obviously be 'const' here. The patch makes them const, though of course doing so breaks the ABI. However, I'd argue that this is in fact an ABI fix and not an ABI break. Our documented policy is that the various *Handle<> types should never be instantiated directly in user code. If you follow that rule, using set_boolean_list() and set_integer_list() becomes impossible without the 'const'. Also, set_boolean_list() was broken anyway to begin with. It is possible to leave the old methods with the non-const parameters in the library, and enclose them within #ifndef GLIBMM_DISABLE_DEPRECATED conditions in the header file. But given the overall brokenness of the array interface before this patch, I'm not sure it's actually worth the hassle.
I know it's tedious, but I think we need to prove (with a test) that each existing method cannot work, and therefore cannot be in use, before we change it. At least the test will prove that this does work.
This will have to wait for the next release cycle.
We should take a look at this for glibmm 2.13.
Committed, including necessary KeyFile fixes: 2007-08-04 Daniel Elstner <daniel.kitta@gmail.com> * containerhandle_shared.h (TypeTraits<bool>): Rewrite completely broken type adapter (bug #406960). * src/keyfile.{ccg,hg}: Fix the implementation to correctly use ArrayHandle<>. Fix compilation with the new ArrayHandle<bool> code.