GNOME Bugzilla – Bug 739778
MSVC compilation issue due to the use of 'round()'
Last modified: 2015-01-05 14:02:24 UTC
Created attachment 290158 [details] [review] Use 'floor' instead of 'round' for MSVC builds 'gdk-pixbuf/io-jpeg.c' has recently introduced a macro called DPCM_TO_DPI (currently at lines 616 and 619). These translate in calls to 'round()'. However, MSVC knows nothing about 'round()'. Its nearest equivalent is probably 'floor()'. I've attached a patch which allows us to continue compiling with MSVC.
Created attachment 290396 [details] [review] Provide Implementation for round() for Visual Studio Hello John, Unfortunately we probably need to look at the situation when we have round(x), when x < 0. In GTK+ 3.x there is the use of a fallback-c89.c (which I did some time ago to address similar issues such as this), so my approach is to use a compatibility header, like what GStreamer does, that is strictly for use with Visual Studio, which would implement round() for pre-2013 Visual Studio. Can you add build/win32/math-compat in your include directories (before the stock Visual Studio headers are included), and try whether the build works for you? With blessings, thank you!
Hi Chun-Wei. I'd be happy to try that for you but there's no such folder AFAICT. Should it be in my gdk-pixbuf folder structure somewhere? Or are you thinking of somewhere else?
Hmmm... I just thought of something. My original patch consisted of a #define to call floor() instead of round() - like so:- #define round(value) floor((value) + 0.5) I'm not sure why I didn't spot this before but (if we assume 'round' to mean 'round to nearest') that #define will surely work for both positive and negative values won't it? Remember that 'floor()' returns the nearest whole number which is lower than the supplied argument (i.e. numerically lower than - not lower in magnitude). Some examples:- Original argument = 2.4 Returned value = floor(2.4 + 0.5) = 2.0 Original argument = 2.6 Returned value = floor(2.6 + 0.5) = 3.0 Original argument = -2.4 Returned value = floor((-2.4) + 0.5) = -2.0 Original argument = -2.6 Returned value = floor((-2.6) + 0.5) = -3.0
If you have "rint" then that would be a reasonable function to use too. (Ditto for "nearbyint", but then we're in C99 territory.) For the nit-pickers out there, there are some numbers for which the floor(x+0.5) hack does not work right: (a) The largest floating point value strictly less than 0.5. The problem is that the addition produces 1 due to rounding. (b) A set of large integers near 2^52 for which adding 0.5 is the same as adding 1, again due to rounding.
rint() isn't available either for MSVC but it looks as if my solution and Chun-wei's solution would both work in the majority of cases. Or can somebody suggest a better solution?
I just updated from git master but still can't build (due to this problem). If neither of the proposed solutions is acceptable, please can someone come up with something more appropriate? Thanks.
Hello Morten, rint(), nearbyint() and round() are all C99 territory, so we need to have fallback implementations for them for pre-Visual Studio 2013 Visual C++. The round() fallback implementation (that I proposed in comment 1) is taken from GTK+ (from gdk/fallback-c89.c and/or gtk/fallback-c89.c). Would the things in there work for you? --- Hello John, Sorry, I did not receive notification as I was not on the CC list for this bug, hence I did not come back to this bug. You will need to include build/win32/math-compat in your list of include directories for your build, and it should include the math.h which would do the necessary work of (1) including the stock Visual Studio math.h, and (2) provide the implementation for round() for pre-Visual Studio 2013 Visual Studio. --- Hi Nacho, Do you mind if you can take a quick look at this? Highly appreciated! With blessings, thank you!
The fallback is fine for gtk+ purposes. Just add a comment that there are a few corner cases where it does not match C99's round function.
Created attachment 293220 [details] [review] Provide Implementation for round() for Visual Studio (comments added) Hello Morten, I have added the comments in the fallback implementation regarding the items that you have mentioned earlier, in this version of the patch. With blessings, thank you!
Review of attachment 293220 [details] [review]: Let's get this in then.
Review of attachment 293220 [details] [review]: Hello Nacho/Morten, Thanks for the comments and ack about the patch, the patch was pushed as a8dc737. With blessings, thank you!
Sorry to be a critic but IMHO this solution is actually a bit clumsy (due to the fact that gdk-pixbuf's internal header has been unfortunately named 'math.h'). This causes it to conflict with the standard VC header. It's possible to avoid this conflict by making sure that gdk-pixbuf's 'build\win32\math-compat' folder gets searched before the standard VC folder. But in VC8 (which I'm using) it's not possible to do this at the project level. It can only get done (reliably) at the IDE level (under "Tools->Options->Projects and Solutions->VC++ Directories"). But if it gets done at the above (IDE) level, ALL projects will #include gdk-pixbuf's version of math.h, in preference to VC's version. Admittedly, our header starts off by #including the official VC header but this is bound to end up causing problems. For example, suppose some other project has also implemented its own version of 'round()'. To be honest, I think my suggestion was a lot safer...
Hi John, There is a setting under each project that is on "additional include directories" under the "C/C++" settings in the project settings. You can see the 2008 projects under build/win32/vs9 for the gdkpixbuf.vcproj(in) on how that is done, so there is no need to include that directory globally for the entire IDE. As far as I know, this should be the same scripts Visual studio versions, as this setting is rather commonly used. In fact, for VS2010 and later, it is made harder for one to modify the global include directories, by purpose. Hope this clears this up for you. With blessings.
Thanks for the prompt reply. You're right to say that each project (in fact, each target) can specify additional include folders - but on my system, those additional folders are getting searched AFTER the main VC include folders. So (even if I add the path to math-compat) gdk-pixbuf's version doesn't get found. VC's version of math.h always gets found first. I can modify that by updating the global IDE folders, rather than the project's folders - but those global settings get applied to all projects (not just gdk-pixbuf). I've a feeling that (in later version of VC++) you can set a project's folders to get searched first. But that doesn't seem to be possible with VC8, from what I can tell. Hope that makes sense.