|
2012-12-19 18:32, Glenn Rice skrev:
I have written a poppler-glibmm wrapper for poppler-glib via mmproc, and I was wondering if someone who is familiar with wrapping glib libraries in this way could take a look at what I have done and check for common pitfalls. Also any suggestions for improvement would be welcome. The code is available in a git repository at git://git.freedesktop.org/git/poppler/poppler-glibmm. The repository is browsable via http://cgit.freedesktop.org/poppler/poppler-glibmm/tree/You will find some comments in the attached file. I have browsed the source code in poppler-glibmm/poppler-glib/src only very quickly. (I haven't even opened all files.) I'm not familiar with poppler-glib. It would have taken me a lot of time to check if the classes and methods are correctly wrapped. The attached file contains some questions. I don't expect answers to those questions. They just show details that I don't know if they are correct. They might well be correct. Kjell |
Some comments on poppler-glibmm, 2012-12-23 ------------------------------- >>> configure.ac AC_INIT([poppler-glibmm], [0.21.1], [https://bugzilla.gnome.org/enter_bug.cgi?product=poppler-glibmm], [poppler-glibmm], [http://www.gtkmm.org/]) Is the bug report URL and project website URL correct? AC_SUBST([POPPLER_GLIBMM_MODULES], ['poppler-glib >= 0.20 glibmm-2.4 >= 2.16 cairomm-1.0 >= 1.10 giomm-2.4 >= 2.16']) glibmm-2.4 and giomm-2.4 version 2.16.0 were released in March 2008. Are you sure poppler-glibmm does not require newer versions? MM_ARG_ENABLE_WARNINGS([POPPLER_GLIBMM_WXXFLAGS], [-Wall], [-pedantic -Wall -Wextra], [G POPPLER_GLIB GLIBMM CAIROMM GTKMM]) should be MM_ARG_ENABLE_WARNINGS([POPPLER_GLIBMM_WXXFLAGS], [-Wall], [-pedantic -Wall -Wextra -Wno-long-long], [G POPPLER_GLIB GLIBMM CAIROMM GTKMM]) Without -Wno-long-long it's not possible to build poppler-glibmm after ./autogen.sh --prefix=xxx --enable-warnings=fatal >>> Makefile.am # Optional: auto-generate the ChangeLog file from the git log on make dist #include $(top_srcdir)/build/dist-changelog.am Uncomment the 'include' line if you want 'make dist' to generate ChangeLog from 'git log'. >>> poppler-glibmm.doap <homepage rdf:resource="http://www.gtkmm.org/"; /> <mailing-list rdf:resource="mailto:gtkmm-list gnome org" /> Is it correct? >>> README This module is part of the GNOME C++ bindings effort <http://www.gtkmm.org/>. Is it correct? >>> codegen/generate_defs_and_docs.sh I added this file fairly recently at http://git.gnome.org/browse/mm-common/tree/skeletonmm/codegen/generate_defs_and_docs.sh Perhaps you want to take a copy and adapt it to poppler-glibmm. >>> codegen/Makefile.am If you add codegen/generate_defs_and_docs.sh, then add this line to Makefile.am: dist_noinst_SCRIPTS = generate_defs_and_docs.sh >>> codegen/generate_extra_defs_poppler-glib.cc g_type_init() is deprecated from glib 2.36 (actually 2.35.1). Here's one way to handle it. #if !GLIB_CHECK_VERSION(2,35,1) g_type_init(); #endif >>> doc/doc-install.pl >>> doc/doc-postprocess.pl >>> doc/doxygen.css >>> doc/tagfile-to-devhelp2.xsl It's not necessary to store these files in the git repository. They are copied from mm-common by mm-common-prepare in autogen.sh, because configure.ac contains MM_CONFIG_DOCTOOL_DIR. >>> poppler-glib/poppler-glibmm.pc.in >>> poppler-glib/poppler-glibmm-uninstalled.pc.in URL: http://www.gtkmm.org/ Is it correct? >>> poppler-glib/poppler-glibmm/wrap_init.cc It's not necessary to store this file in the git repository. It's generated by generate_wrap_init.pl when poppler-glibmm is built, thanks to include $(top_srcdir)/build/generate-binding.am in poppler-glib/src/Makefile.am. >>> poppler-glib/src/poppler-glibmm_extra.defs I've changed the name of this file in mm-common/skeletonmm/skeleton/src to skeleton_signal.defs. I seems more appropriate, since it contains information on signals and properties. I've also added skeleton_vfunc.defs to mm-common/skeletonmm/skeleton/src. If there are virtual functions in poppler-glib that you want to wrap in poppler-glibmm, you will need such a file. Unfortunately it must be written manually. >>> poppler-glib/src/index_iter.hg and other .hg files inline operator bool() const { return gobj() != NULL; }; In gtkmm 'operator bool()' has been replaced by /** This typedef is just to make it more obvious that * our operator const void* should be used like operator bool(). */ typedef const void* BoolExpr; /** Tests whether the IconInfo is valid. * For instance, * @code * if(iconinfo) * do_something() * @endcode */ operator BoolExpr() const; IconInfo::operator const void*() const { return gobj() ? GINT_TO_POINTER(1) : 0; } A drawback with operator bool() is that a bool can be implicitly converted to int. std::basic_ios<> contains operator void*() instead of operator bool(), probably to avoid implicit conversion to int. In C++11 std::basic_ios<> contains explicit operator bool() const; Unfortunately operators can't be declared explicit unless the compiler supports C++11 features. operator void*() also has its problems. Pointers can be compared to each other. 'if (object1 < object2)' is allowed, if the objects can be converted to void*. To disallow such nonsense comparisons, all relational operators are declared private without implementation in http://git.gnome.org/browse/glibmm/tree/glib/src/variant.hg >>> poppler-glib/src/form_field.hg and other .hg files You declare all member functions const, even functions such as void text_set_text(const Glib::ustring& text) const which presumably changes the underlying C structure. I suppose this is a function which is physically const, but logically non-const. I would not declare such functions const, even though the compiler allows it. >>> General comments The reference documentation may or may not look good, depending on which versions of mm-common and doxygen you use. This is discussed in bug https://bugzilla.gnome.org/show_bug.cgi?id=673984 I recommend that you use mm-common 0.9.6 or later and doxygen 1.8.0 or later. I can't find a single example of 'Glib::RefPtr<const xxx>', although there are many 'Glib::RefPtr<xxx>'. That may or may not be correct. I haven't studied poppler-glib(mm)'s classes and methods carefully enough to know. If you're in doubt you should read what's said about RefPtr and constness in the gtkmm tutorial at http://developer.gnome.org/gtkmm-tutorial/stable/sec-refptr-constness.html.en It's very easy to get 'refreturn' wrong in _WRAP_METHOD(). I checked just two examples in your code, one with and one without refreturn. Both were correct. Obviously you have thought about that, but don't be surprised if errors show up in the future. Many such bugs have been detected in glibmm and gtkmm. Either a missing refreturn which may cause an object to be deleted while there is still a pointer to it, or an extra refreturn which causes a memory leak.