After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 739778 - MSVC compilation issue due to the use of 'round()'
MSVC compilation issue due to the use of 'round()'
Status: RESOLVED FIXED
Product: gdk-pixbuf
Classification: Platform
Component: general
git master
Other Windows
: Normal normal
: ---
Assigned To: gdk-pixbuf-maint
gdk-pixbuf-maint
Depends on:
Blocks:
 
 
Reported: 2014-11-07 12:53 UTC by John E
Modified: 2015-01-05 14:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Use 'floor' instead of 'round' for MSVC builds (748 bytes, patch)
2014-11-07 12:53 UTC, John E
none Details | Review
Provide Implementation for round() for Visual Studio (4.76 KB, patch)
2014-11-11 07:07 UTC, Fan, Chun-wei
none Details | Review
Provide Implementation for round() for Visual Studio (comments added) (5.21 KB, patch)
2014-12-23 05:05 UTC, Fan, Chun-wei
committed Details | Review

Description John E 2014-11-07 12:53:41 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.
Comment 1 Fan, Chun-wei 2014-11-11 07:07:38 UTC
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!
Comment 2 John E 2014-11-11 11:54:43 UTC
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?
Comment 3 John E 2014-11-25 12:12:21 UTC
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
Comment 4 Morten Welinder 2014-12-02 14:30:11 UTC
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.
Comment 5 John E 2014-12-06 11:32:09 UTC
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?
Comment 6 John E 2014-12-20 11:43:19 UTC
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.
Comment 7 Fan, Chun-wei 2014-12-22 04:18:23 UTC
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!
Comment 8 Morten Welinder 2014-12-22 17:06:41 UTC
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.
Comment 9 Fan, Chun-wei 2014-12-23 05:05:18 UTC
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!
Comment 10 Ignacio Casal Quinteiro (nacho) 2014-12-23 07:14:02 UTC
Review of attachment 293220 [details] [review]:

Let's get this in then.
Comment 11 Fan, Chun-wei 2014-12-23 08:07:42 UTC
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!
Comment 12 John E 2015-01-05 10:36:38 UTC
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...
Comment 13 Fan, Chun-wei 2015-01-05 12:48:38 UTC
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.
Comment 14 John E 2015-01-05 14:02:24 UTC
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.