Several float to int potential rounding errors/inconsistencies

[Lubomir I. Ivanov]

On 11 March 2017 at 17:24, Dirk Hohndel <dirk at hohndel.org> wrote:
> 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 think they've released WinXP 64bit at some point

probably best to stay with the 32bit as the main one for now.
at least, that's the standard practice with other Win32 software - if
you provide one build use 32bit, but you can also provide a 64bit
build next to it.

the 64bit build for 64bit OSes might increase performance a little and
can support more than 4GB of RAM.
but other than that there are no noticeable benefits.

lubomir
--