GNOME Bugzilla – Bug 517530
additional includes for C functions in several files for Solaris
Last modified: 2008-02-29 17:56:47 UTC
I just finished building gtkmm 2.12.4 on x86_64-sun-solaris2.10 with the Sun Workshop 12 bundle. Several files, especially in the examples and demo, require additional #include directives to get prototypes for certain functions (memset(), strlen(), atoi(), floor(), et. al.) and several require the define for M_PI, which is in math.h on Solaris. Attached is the patch that fixes each of these files. I'm not currently wrapping any of the #include statements that I added in any kind of #ifdef guard, because gtkmm's configure currently isn't doing much to check for header files. That could be changed, if any of these are problematic.
Created attachment 105603 [details] [review] add include directives necessary to get prototypes for common C functions
In general, there's no need to include both <cstring> and <string.h> -- they should be identical except one is supposed to import the functions into the std:: namespace. So we should probably either qualify the strcmp() calls with thd std namespace or change the cstring include to string.h, not include both cstring and string.h. Also, I'm afraid you patched the wrong files (which is understandable since it appears that you're patching the tarball). But fyi, the files under gtk/gtkmm/ are for the most part generated files. The 'source' files that will need to be patched are in gtk/src/
Ah yes, I had forgotten about the files being generated. I'll happily fix up the patch, if there's consensus on what direction to take.
Including C headers such as <*.h> is not quite the C++ way. The whole point of namespace is to avoid name clash. I suggest adding missing "std::" prefixes to affected calls, or "using std::<function>;" when the former is not viable, and avoid "using namespace std;". OTOH, it's suspicious that gcc on Linux can circumvent "std::", which might worth a bug on gcc side.
> I'll happily fix up the patch, if there's consensus on what direction to take. I wouldn't object to the use of std::, but we already use C functions regularly without prefixes so it's not worth bothering about too much. I'll take any patch that fixes the build.
Created attachment 105642 [details] [review] header_fix_for_gcc4.3_and_solaris.patch "std::" prefix version based on Tim Mooney's patch. Some notes: 1. getc_unlocked() used in demos/gtk-demo/demowindow.cc is POSIX only we have to use #include <stdio.h> 2. several <cmath> inclusion are based on Tim Mooney's patch, without which doesn't fail to build with gcc4.3 on Linux, examples are demos/gtk-demo/example_textview.cc, demos/gtk-demo/example_treeview_editable_cells.cc, examples/book/drawingarea/arcs/myarea.cc, examples/book/drawingarea/clock/clock.cc 3. It's likely I was missing something. Tim Mooney please check.
I'll attach a patch for the stuff that was missed, but unfortunately your changes relating to <cmath> don't completely work on Solaris when building with STLport4, which is what nearly everyone on Solaris will do. Including <cmath> gets the std::abs(), std::floor(), and other functions that didn't have prototypes before, but unfortunately the define for M_PI is not present in STLport4's <cmath>. Other than defining it ourselves, it seems that the only way to get it on Solaris when using STLport4 is to include <math.h>. With that in mind, how do we handle M_PI?
The path of least resistance would be to just include math.h instead of cmath. That seems to be the most portable solution.
Created attachment 105673 [details] [review] add std:: for exit and strtod This patches applies after Deng's patch. It adds a std:: to exit and strtod. It also updates the gtk/README to give a tip on how to rebuild the gtkmm files from the source files. It still doesn't resolve the problem with some of the examples needing math.h on Solaris for M_PI
I just thought of something: I believe glib actually provides a G_PI symbol for precisely these portability reasons. Maybe we should just switch everything from M_PI to G_PI. http://library.gnome.org/devel/glib/stable/glib-Numerical-Definitions.html#id2975494
We would still need to include <cmath> and would also then need <gtype.h>, but your idea seems reasonable to me.
After consulting on IRC #gcc@irc.oftc.net, Andrew Pinski pointed out there is a gcc bug on this: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=6257 , and it is essentially a defect against C++ (http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#456), which has already been solved by stating "It is unspecified whether these names are first declared within the global namespace scope and are then injected into namespace std by explicit using-declarations (7.3.3 [namespace.udecl])." So now both GCC and solaris has the correct behavior. And I guess it's still more likely to favor the std:: prefix solution.
Created attachment 105679 [details] [review] gcc-4.3-and-solaris-header-fix.patch Reverted 2 useless dirty hack from my original patch (didn't search for M_PI), and waiting for the solution of G_PI. Tim's patch should still appliable after this one. And one more question(should've asked before) : Are gtk/gtkmm/targetentry.{h,cc} generated from gtk/src? I can't find original *.{h,cc}g files for them.
Please don't confuse me with multiple patches that have to be applied in order. Please try to agree on one patch.
Created attachment 105700 [details] [review] merged-refined-gcc4.3-solaris-fix.patch Merge Tim's patch and refined to use using directive. Some notes: 1. Now it uses i.e. "using std::strlen;" form instead of prefixing every C function, as C++ standard doesn't specify where C functions should locate first, making direct C function calls conformant per se. So just let using directives fix compiler specific issues. 2. "#include <math.h>" is used instead of <cmath>, as noted in examples/gdk/radar.cc that <math.h> is "Needed by the IRIX MipsPro compiler, for sin" and other certain reason. Together with M_PI issue, #include <math.h> seems to be the simplest workaround. Not fully tested yet. Tim please check.
Your merged patch work well on Solaris. I would say the merged-refined-gcc4.3-solaris-fix.patch should replace and obsolete all the previous patches for this bug, and it can be applied by anyone with commit access. Thanks!
Thanks. I have committed this to gtkmm's trunk and gtkmm-2-12 branches, and to gtkmm-documentation. It was not an svn patch so there were some conflicts. Hopefully I have resolved them appropriately, but please reopen this patch if there is still a problem. I did not apply the unrelated and rather odd change to the README.
I made that change to the gtk/README because for the casual user of gtkmm, it's not at all obvious how to rebuild the generated files from the source files. This is the second time I've provided build patches for gtkmm, and each time I've had to spend time figuring out how to rebuild the gtk/gtkmm files. I'm sure I'm not the only person that's wasted time trying to figure it out. To save myself and others that time in the future, I thought it would be nice if the process were documented for the uninitiated. If there's a better way to regenerate the files, then great, let's document it, but either way, the process needs to be documented, and gtk/README seems like the obvious spot.
In general, it will work if you run autogen.sh. If you are lucky, it will work if you use --enable-maintainer-mode with configure, I think.
Created attachment 106206 [details] [review] a better patch for the gtk/README, on how to rebuild the gtkmm files Ok, how about this patch to the README?
OK, thanks. I have added this text: The .h/.cc files are usually only built from the .hg/.ccg files when --enable-maintainer-mode has been passed to configure. This will be the case when building from svn, when using the autogen.sh script. If you think that you need to change the .hg/ccg files then you should indeed be using an svn checkout so that you can create an svn patch and submit it to the gtkmm project.
Thanks, that will work!