ossama-othman / MaRC

MaRC - Map Reprojections and Conversions
GNU Lesser General Public License v2.1
1 stars 0 forks source link

Implicit integer to floating point conversion triggers warning #111

Open ossama-othman opened 4 years ago

ossama-othman commented 4 years ago

Describe the bug Building MaRC with clang++ 10 or better triggers a warning diagnostic at compile-time related to implicit conversion of a 64 bit integer (int64_t) to a double floating point value:

In file included from ../lib/marc/scale_and_offset.h:15:
../lib/marc/details/scale_and_offset.h:120:40: warning: implicit conversion
      from 'const long' to 'double' changes value from 9223372036854775807
      to 9223372036854775808 [-Wimplicit-int-float-conversion]
                else if (max * scale > T_max)
                                     ~ ^~~~~
In file included from ../lib/marc/extrema.h:15:
../lib/marc/Map_traits.h:106:41: warning: implicit conversion from 'long' to
      'const double' changes value from 9223372036854775807 to
      9223372036854775808 [-Wimplicit-int-float-conversion]
            constexpr double type_max = std::numeric_limits<T>::max();
                             ~~~~~~~~   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Scale_Offset_Test.cpp:38:33: warning: implicit conversion from 'long' to
      'const double' changes value from 9223372036854775807 to
      9223372036854775808 [-Wimplicit-int-float-conversion]
    constexpr double T_max    = std::numeric_limits<T>::max();
                     ~~~~~      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The crux of the problem is that MaRC "reads" all data from the source images as double values. This isn't a problem for 8, 16, and 32 bit integers since their maximum values can be stored in a 64 bit double without loss of precision since the significand of a double value has enough bits to store it (e.g. IEEE 754 double is 53 bits wide). However, that isn't the case for 64 bit integers since the largest 64 bit value is obviously won't fit into the smaller double type signficand (e.g. 53 bits).

To Reproduce Steps to reproduce the behavior:

  1. ./configure CXX=clang++
  2. make check

Expected behavior No warning related to implicit conversion of an integer to a floating point value should be triggered.

Desktop (please complete the following information):

ossama-othman commented 4 years ago

Related bug documented in lib/marc/map_parameters.h:

        /**
         * @brief Maximum valid physical value.
         *
         * @note This value corresponds to the %FITS "DATAMAX"
         *       keyword.
         *
         * @bug On platforms that implement the IEEE 754 floating
         *      point standard, the use of @c double as the underlying
         *      @c DATAMAX type will cause loss of precision if the
         *      %FITS @c DATAMAX values is an integer that requires
         *      more than 53 bits since the significand of a 64 bit
         *      IEEE 754 floating point value is only 53 bits wide.
         *      Special handling will be need to be implemented to
         *      handle such a case.
         */
        MaRC::optional<double> datamax_;
ossama-othman commented 4 years ago

The warning could be silenced by using an explicit cast, but that would only hide the problem in this case. Ideally the code should be changed so that no cast is necessary.