Warning C4244 with Gmpzf_type and Gmpfr_type on MS Visual Studio

A-Louarn commented 4 years ago

Issue Details

Hi, Including a GMP-based kernel into a project with Visual Studio yields a few warnings (C4244) about possible loss of data:

1>[…]\vcpkg\installed\x64-windows\include\CGAL/GMP/Gmpzf_type.h(452,25): warning C4244: 'initializing': conversion from 'mp_bitcnt_t' to 'unsigned long', possible loss of data
1>[…]\vcpkg\installed\x64-windows\include\CGAL/GMP/Gmpfr_type.h(1011,19): warning C4244: 'initializing': conversion from 'mp_bitcnt_t' to 'long', possible loss of data
1>[…]\vcpkg\installed\x64-windows\include\CGAL/GMP/Gmpfr_type.h(1136,54): warning C4244: 'argument': conversion from 'mpir_ui' to 'unsigned long', possible loss of data
1>[…]\vcpkg\installed\x64-windows\include\CGAL/GMP/Gmpfr_type.h(1138,54): warning C4244: 'argument': conversion from 'mpir_ui' to 'unsigned long', possible loss of data

These warnings arise with the vcpkg version of CGAL, but as the offending code is identical between the vcpkg include files and the ones on this repo, I would guess that they also appear on the latest versions of CGAL too.

Source Code

#include <iostream>

#include <CGAL/Exact_predicates_exact_constructions_kernel.h>

int main(int argc, char ** argv)
{
    return EXIT_SUCCESS;
}

Environment

Operating system Windows 64 bits
Compiler: MSVC 16.4.2
Release or debug mode:
Specific flags used (if any):
CGAL version:CGAL version: 5.00.0 (build 090)
Boost version:
Other libraries versions if used (Eigen, TBB, etc.):

lrineau commented 4 years ago

@mglisse Do you think we could:

check that the casts are safe,
then add the static_cast to quiet the compiler?

mglisse commented 4 years ago

1>[…]\vcpkg\installed\x64-windows\include\CGAL/GMP/Gmpzf_type.h(452,25): warning C4244: 'initializing': conversion from 'mp_bitcnt_t' to 'unsigned long', possible loss of data

We could use mp_bitcnt_t (not sure in which releases of GMP and MPIR this typedef appeared) for variable zeros, but that would likely move the warning to e += zeros 3 lines below. The limiting factor is that we are using long as the exponent type. We could try using long long on win64, although that means checking all the uses of long in a few files, and we are likely to hit the fact that GMP does not support long long... As long as we are using long and don't check for overflow, we don't care about this warning and any way to disable it is fine. For Gmpfr, we could use mpfr_exp_t or mpfr_prec_t in more places, but again, with GMP/MPFR using long, conversions are necessary, so trying to handle numbers whose exponent reaches 32 bits on win64 does not look worth the effort. So yes, if you want to disable the warning in these files, that looks fine to me.

We could also try not including those 2 files when nothing needs them, but that looks like a deep rabbit hole...

CGAL / cgal