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

{MSVC} Building with /W4 results in warning C4127 #17

Closed
asmaloney opened this issue Jan 16, 2023 · 4 comments
Closed

{MSVC} Building with /W4 results in warning C4127 #17

asmaloney opened this issue Jan 16, 2023 · 4 comments

Comments

@asmaloney
Copy link

...
\...\extern\CRCpp\inc\CRC.h(904): warning C4127: conditional expression is constant
\...\extern\CRCpp\inc\CRC.h(501): note: see reference to function template instantiation 'CRCType CRC::CalculateRemainder<CRCType,32>(const void *,size_t,const CRC::Table<CRCType,32> &,CRCType)' being compiled
        with
        [
            CRCType=uint32_t
        ]
...

Right above line 904, an MSVC warning is explicitly disabled. I wonder if the same should be done here?

Happy to create a PR if so.

Workaround is to disable it for the entire include:

#if defined( WIN32 ) || defined( _WIN32 ) || defined( WINCE )
// Disable warning about "conditional expression is constant".
#pragma warning( push )
#pragma warning( disable : 4127 )
#endif
#include "CRC.h"
#if defined( WIN32 ) || defined( _WIN32 ) || defined( WINCE )
#pragma warning( pop )
#endif
@d-bahr
Copy link
Owner

d-bahr commented Jan 18, 2023

There are a couple of places where this is the case, I think, not just line 904. if constexpr would be the C++17 way of solving this but would still break for older compilers.

I think disabling the warning for the whole file is acceptable. I'll fix it shortly.

@asmaloney
Copy link
Author

👍 I found another solution in case you want another option.

You can assign to a var and then check it like this (from my own code):

   // Assign to var to avoid MSVC warning
   // This section can be simplified in C++17 using a "constexpr if".
   constexpr bool cSizeCheck = ( sizeof( size_t ) >= sizeof( int64_t ) );
   if ( cSizeCheck )
...

MSVC only complains if it's in the conditional expression.

Thanks Daniel!

d-bahr added a commit that referenced this issue Jan 18, 2023
Added conditional compilation blocks to disable C4127 when compiling with MSVC.
@d-bahr
Copy link
Owner

d-bahr commented Jan 18, 2023

Decided to go with the #ifdef solution. If compiled with C++03 then the constexpr solutions may not work.

MSVC should be smart enough to optimize out the branches that are not invoked. In any case, issue closed.

@d-bahr d-bahr closed this as completed Jan 18, 2023
@asmaloney
Copy link
Author

If compiled with C++03 then the constexpr solutions may not work.

Ah right. Could use static instead.

Anyways - thanks for the great lib!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants