exiv2
exiv2 copied to clipboard
Add Safe::cast<T>
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 Report
Merging #898 into master will increase coverage by
0.03%
. The diff coverage is100%
.
@@ 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.
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
-
is
type
correct on line 348? For sure, I don't know what it means and apparently the compiler doesn't either! -
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. -
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.
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
- 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.
- 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.
- 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
Let's see if we can get this magic into v1.00.