Several float to int potential rounding errors/inconsistencies

[Dirk Hohndel]

On Sat, Mar 11, 2017 at 03:41:53PM +0200, Lubomir I. Ivanov wrote:
> On 11 March 2017 at 07:21, Jérémie Guichard <djeBrest at gmail.com> wrote:
> > Hey guys,
> >
> > I've added the flag conditionally as discussed in previous mail, and updated
> > the pull request.
> 
> i've tested the changes locally on Windows and it builds fine.
> llrint() is also available.
> 
> 4.9.0 seems to be the version they've added "float-conversation", so
> this is correct:
> 
> + if (NOT CMAKE_CXX_COMPILER_VERSION VERSION_LESS "4.9.0")
> + set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wfloat-conversion")
> + endif()
> 
> of course, there is no need of lrint() in a lot of locations and
> rint() can be used instead, like here:
> 
> - sample->depth.mm = depth * FEET * 1000;
> + sample->depth.mm = lrint(depth * FEET * 1000);

I find changes like this not only unnecessary but also potentially
confusing.

> here, t1 is of type int as well:
> + int t1 = lrint(max_d / slope);

> but that's not a big issue as long as numbers are not truncated for
> 64bit builds.
> our main Windows build is 32bit, so this is pretty much in effect:
> sizeof(long) == sizeof(int).

I keep wondering at which point it would make sense to switch to 64bit
builds for Windows as well. Our stats still show a small but fairly stable
share of 32bit systems running Subsurface. But then again, we also have a
few people on 4.5 running Windows XP :-)

> i haven't tested if the changes break functionality in some way, hopefully not.

If we had better tests, we could use those to test that nothing breaks...

/D