Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Safe::cast<T> #898

Open
wants to merge 1 commit into
base: old-master
Choose a base branch
from
Open

Add Safe::cast<T> #898

wants to merge 1 commit into from

Conversation

D4N
Copy link
Member

@D4N D4N commented Jun 4, 2019

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
Copy link

codecov bot commented Jun 5, 2019

Codecov Report

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

Impacted file tree graph

@@            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%> (ø) ⬆️
unitTests/test_safe_op.cpp 100% <100%> (ø) ⬆️

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
Copy link
Collaborator

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.

src/safe_op.hpp Outdated Show resolved Hide resolved
src/safe_op.hpp Outdated Show resolved Hide resolved
src/safe_op.hpp Outdated Show resolved Hide resolved
cast(U value)
{
// U signed, T unsigned => T_MAX < U_MAX
return (value <= std::numeric_limits<T>::max()) && (value >= std::numeric_limits<T>::min())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is possible that T_MAX > U_MAX here, because this code could be used for casts like int8_t to uint32_t. I think the code already works correctly, because value will get automatically promoted to type T in the comparison. But I would recommend swapping the order to first check that value isn't negative:

0 <= value && value <= std::numeric_limits<T>::max()

piponazo
piponazo previously approved these changes Jun 18, 2019
Copy link
Collaborator

@piponazo piponazo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I do not have much experience with c++ template programming, I took a quick look and every looks good to me. It's great to see that you are using TYPED_TEST to have a nice set of tests for the new functions.

@mergify mergify bot dismissed piponazo’s stale review August 2, 2019 21:45

Pull request has been modified.

@D4N
Copy link
Member Author

D4N commented Aug 2, 2019 via email

Comment on lines +36 to +41

// MSVC is stupid and pollutes the global namespace with max() and min() macros
// that break std::numeric_limits<T>::max() and min()
#undef max
#undef min

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually already defined in cmake/compilerFlags.cmake. Although it seems it is only defined for MSVC. @D4N maybe you had issues with MinGW ?

Base automatically changed from master to old-master February 28, 2021 05:57
@clanmills
Copy link
Collaborator

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

@clanmills clanmills added this to the v1.00 milestone Apr 13, 2021
@clanmills clanmills added the refactoring Cleanup / Simplify code -> more readable / robust label Apr 13, 2021
@clanmills clanmills mentioned this pull request Apr 14, 2021
@kevinbackhouse kevinbackhouse modified the milestones: v0.28.0, Backlog Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Cleanup / Simplify code -> more readable / robust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants