Add Safe::cast<T>

[D4N]

This PR adds a new function into the Safe namespace: cast<T>. It is intended to convert a integer from one type to the other without causing over- or underflows.

Thanks to some ternary operator abuse it is even completely constexpr even with C++11, although readability suffers a little.

Please review this PR with more care, I've created it in a pretty sleep deprived state.

[codecov[bot]]

Codecov Report

Merging #898 into master will increase coverage by 0.03%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #898      +/-   ##
==========================================
+ Coverage   71.09%   71.12%   +0.03%     
==========================================
  Files         148      148              
  Lines       19376    19400      +24     
==========================================
+ Hits        13775    13799      +24     
  Misses       5601     5601

Impacted Files	Coverage Δ
src/safe_op.hpp	100% <100%> (ø)	:arrow_up:
unitTests/test_safe_op.cpp	100% <100%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 98e63e4...65591ca. Read the comment docs.

[clanmills]

Gosh, Dan. That's amazing code and I have no idea what it does!

I'm going to leave it to @piponazo to approve (although I will approve if you ask me).

I got it to built and pass the test suite with msvc 2017/Release and 2019/Release (with a little code change).

When I built, I got this error:

  pngchunk_int.cpp                                                                                                                                                                                                                               
c:\msys64\home\rmills\gnu\github\exiv2\master\src\safe_op.hpp(347): error C2064: term does not evaluate to a function taking 0 arguments (compiling source file C:\msys64\home\rmills\gnu\github\exiv2\master\src\pngchunk_int.cpp) [C:\msys64\h 
ome\rmills\gnu\github\exiv2\master\build\src\exiv2lib_int.vcxproj]                                                                                                                                                                               
c:\msys64\home\rmills\gnu\github\exiv2\master\src\safe_op.hpp(348): error C2064: term does not evaluate to a function taking 0 arguments (compiling source file C:\msys64\home\rmills\gnu\github\exiv2\master\src\pngchunk_int.cpp) [C:\msys64\h 
ome\rmills\gnu\github\exiv2\master\build\src\exiv2lib_int.vcxproj]                                                                                                                                                                               
c:\msys64\home\rmills\gnu\github\exiv2\master\src\safe_op.hpp(348): error C2039: 'type': is not a member of 'std::enable_if<false,void>' (compiling source file C:\msys64\home\rmills\gnu\github\exiv2\master\src\pngchunk_int.cpp) [C:\msys64\h 
ome\rmills\gnu\github\exiv2\master\build\src\exiv2lib_int.vcxproj]                                                                                                                                                                               
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\type_traits(227): note: see declaration of 'std::enable_if<false,void>' (compiling source file C:\msys64\home\rmills\gnu\github\exiv2\master\src\pngchunk_int.cpp)                                                                                                                                                                                                                           
c:\msys64\home\rmills\gnu\github\exiv2\master\src\safe_op.hpp(348): error C2146: syntax error: missing '>' before identifier 'type' (compiling source file C:\msys64\home\rmills\gnu\github\exiv2\master\src\pngchunk_int.cpp) [C:\msys64\home\r 
mills\gnu\github\exiv2\master\build\src\exiv2lib_int.vcxproj]  

My "fix" is to comment off some code in safe_op.hpp

    namespace Internal
    {
        template <typename from, typename to, typename = void>
        struct is_safely_convertible : std::false_type
        {
            static_assert(std::is_integral<from>::value&& std::is_integral<to>::value,
                          "from and to must both be integral types");
        };
/*
        template <typename from, typename to>
        struct is_safely_convertible<
            from, to,
            typename std::enable_if<((std::numeric_limits<from>::max() <= std::numeric_limits<to>::max()) &&
                                     (std::numeric_limits<from>::min() >= std::numeric_limits<to>::min()))>::type>
            : std::true_type
        {
        };
*/

My reason to try this

You have two definitions for the template:

template <typename from, typename to, typename = void>
and
template <typename from, typename to>

This might be ambiguous.

Other ideas:

I tried some other guesses and none of them compiled. For example, I changed ::type to ::typename because it complained:

safe_op.hpp(348): error C2146: syntax error: missing '>' before identifier 'type'

My Suggestions

1. is type correct on line 348? For sure, I don't know what it means and apparently the compiler doesn't either!

2. Only compile template <typename from, typename to> .... }; for #ifndef _MSC_VER. You'll loose safety for MSVC builds. It could be a compiler bug that goes away in the future. However it's there with both 2017 and 2019.

3. It could be an SDK issue. For both 2017 and 2019, CMake reports:

Selecting Windows SDK version 10.0.17763.0 to target Windows 10.0.17134.

I suspect a lot of the template voodoo comes from the SDK and not the compiler. As CMake's announcing the SDK, it's likely that we could force him to use a different SDK with different consequences.

[D4N]

Robin Mills [email protected] writes:

Gosh, Dan. That's amazing code and I have no idea what it does!

That's usually a bad sign.

I'm going to leave it to @piponazo to approve (although I will approve if you ask me).

I got it to built and pass the test suite with msvc 2017/Release and 2019/Release (with a little code change).

When I built, I got this error:

  pngchunk_int.cpp                                                                                                                                                                                                                               
c:\msys64\home\rmills\gnu\github\exiv2\master\src\safe_op.hpp(347): error C2064: term does not evaluate to a function taking 0 arguments (compiling source file C:\msys64\home\rmills\gnu\github\exiv2\master\src\pngchunk_int.cpp) [C:\msys64\h 
ome\rmills\gnu\github\exiv2\master\build\src\exiv2lib_int.vcxproj]                                                                                                                                                                               
c:\msys64\home\rmills\gnu\github\exiv2\master\src\safe_op.hpp(348): error C2064: term does not evaluate to a function taking 0 arguments (compiling source file C:\msys64\home\rmills\gnu\github\exiv2\master\src\pngchunk_int.cpp) [C:\msys64\h 
ome\rmills\gnu\github\exiv2\master\build\src\exiv2lib_int.vcxproj]                                                                                                                                                                               
c:\msys64\home\rmills\gnu\github\exiv2\master\src\safe_op.hpp(348): error C2039: 'type': is not a member of 'std::enable_if<false,void>' (compiling source file C:\msys64\home\rmills\gnu\github\exiv2\master\src\pngchunk_int.cpp) [C:\msys64\h 
ome\rmills\gnu\github\exiv2\master\build\src\exiv2lib_int.vcxproj]                                                                                                                                                                               
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\type_traits(227): note: see declaration of 'std::enable_if<false,void>' (compiling source file C:\msys64\home\rmills\gnu\github\exiv2\master\src\pngchunk_int.cpp)                                                                                                                                                                                                                           
c:\msys64\home\rmills\gnu\github\exiv2\master\src\safe_op.hpp(348): error C2146: syntax error: missing '>' before identifier 'type' (compiling source file C:\msys64\home\rmills\gnu\github\exiv2\master\src\pngchunk_int.cpp) [C:\msys64\home\r 
mills\gnu\github\exiv2\master\build\src\exiv2lib_int.vcxproj]  

My "fix" is to comment off some code in safe_op.hpp

    namespace Internal
    {
        template <typename from, typename to, typename = void>
        struct is_safely_convertible : std::false_type
        {
            static_assert(std::is_integral<from>::value&& std::is_integral<to>::value,
                          "from and to must both be integral types");
        };
/*
        template <typename from, typename to>
        struct is_safely_convertible<
            from, to,
            typename std::enable_if<((std::numeric_limits<from>::max() <= std::numeric_limits<to>::max()) &&
                                     (std::numeric_limits<from>::min() >= std::numeric_limits<to>::min()))>::type>
            : std::true_type
        {
        };
*/

This is unfortunately not really a fix, as it just comments out the problematic part. The compiler error goes away, but the intended functionality is lost.

My reason to try this

You have two definitions for the template:

template <typename from, typename to, typename = void>
and
template <typename from, typename to>

This might be ambiguous.

That's kind of the point. What I'm doing here is called SFINAE (Substitution Failure Is Not An Error), which means that the compiler does not error out immediately when fails to insert a type in a template overload.

Let's say you have two template overloads of the function foo:

template <typename T, typename = void> void foo()

and

template <typename T> void foo<T, typename something_that_depends_on_T<T>::type>()

When you instantiate foo, the compiler will start inserting the template parameters starting with the most specialized overload (here the second one). It will insert T and then try to find out what something_that_depends_on_T<T>::type is. If the resolution of this type results in an error, then this is not fatal. The compiler simply tries the next less specialized template overload.

You can use this in conjunction with "dummy" template parameters (template parameters that have a unused default value and are not actually used in the template) to have multiple specializations of the same function. You then provide a conditional type (something_that_depends_on_T<T>::type in the above example) that resolves only in the case when you want the overload and doesn't either.

Other ideas:

I tried some other guesses and none of them compiled. For example, I changed ::type to ::typename because it complained:

safe_op.hpp(348): error C2146: syntax error: missing '>' before identifier 'type'

Yes, that cannot work, because type is a public member of std::enable_if.

My Suggestions

1. is type correct on line 348? For sure, I don't know what it means and apparently the compiler doesn't either!

It is definitely correct. GCC and clang don't complain about this and work without problems.

2. Only compile template <typename from, typename to> .... }; for #ifndef _MSC_VER. You'll loose safety for MSVC builds. It could be a compiler bug that goes away in the future. However it's there with both 2017 and 2019.

Not including the overload for MSVC seriously cripples the overall usefulness of the whole code. I'd prefer not to do that unless MSVC really doesn't cooperate.

3. It could be an SDK issue. For both 2017 and 2019, CMake reports:

Selecting Windows SDK version 10.0.17763.0 to target Windows 10.0.17134.

I suspect a lot of the template voodoo comes from the SDK and not the compiler. As CMake's announcing the SDK, it's likely that we could force him to use a different SDK with different consequences.

Unlikely, as this uses only std::enable_if and std::numeric_limits (the former can be implemented in ~5 lines of code and the latter is also very simple, albeit requiring a lot of boilerplate code).

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/Exiv2/exiv2/pull/898#issuecomment-500241656

[clanmills]

Let's see if we can get this magic into v1.00.