weidai11 / cryptopp

free C++ class library of cryptographic schemes
https://cryptopp.com
Other
4.83k stars 1.49k forks source link

'Variable is not used' warnings in VS 2022 #1185

Closed irwir closed 1 year ago

irwir commented 1 year ago

Cryptopp master branch, Visual Studio 2022. There were multiple warning when compilng a project with cryptopp library:

Warning C5264 'CryptoPP::INFINITE_TIME': 'const' variable is not used \cryptopp\cryptlib.h 130

This is because the variable gets defined every time the header was included, A simple fix is to move the definition of CryptoPP::INFINITE_TIME to cryptlib.cpp while the header file has only this declaration: extern const unsigned long INFINITE_TIME;

noloader commented 1 year ago

Thanks @irwir.

This looks like a useless warning from Microsoft tooling. I'm inclined to disable it.

extern const unsigned long INFINITE_TIME;

I don't want to allocate a storage location and then reference it. The only time that is needed when we need to take the address of a value, like ptr = &INFINITE_TIME. Instead, I want the value to be inlined when needed.

How does this look to you:

$ git diff
diff --git a/cryptlib.h b/cryptlib.h
index 5d8792d6..7f07e018 100644
--- a/cryptlib.h
+++ b/cryptlib.h
@@ -127,7 +127,10 @@ enum CipherDir {
        DECRYPTION};

 /// \brief Represents infinite time
-const unsigned long INFINITE_TIME = ULONG_MAX;
+/// \sa https://github.com/weidai11/cryptopp/issues/1185^M
+#ifndef INFINITE_TIME^M
+# define INFINITE_TIME ULONG_MAX^M
+#endif^M

 // VC60 workaround: using enums as template parameters causes problems
 /// \brief Converts an enumeration to a type suitable for use as a template parameter

I despise giving up the the benefit of scope. But I don't see a better way given the constraints (and the useless warning).

noloader commented 1 year ago

@irwir,

I despise giving up the the benefit of scope.

The warning complains about just const. Can you try building with static const. If that does not work, can you try with just static? (I would really like to keep INFINITE_TIME in the CryptoPP namespace).

irwir commented 1 year ago

INFINITE_TIME variable is never used in the library or mentioned in comments. Hence the warnings are valid and should not be ignored or disabled. An external variable should be safe enough (and thread safe) because of being a constant.

static unsigned long INFINITE_TIME = ULONG_MAX; compiles quietly, but makes a modifiable value.

define INFINITE_TIME ULONG_MAX

is a good option if the value has to be in the library.

noloader commented 1 year ago

Thanks @irwir.

Did static const achieve the goal of squashing the warning?

noloader commented 1 year ago

@irwir,

Here's another possibility. Visual Studio 2017 support's C++17's inline variable. We may be able to declare the variable as inline. Once declared inline, it behaves like an inlined function. We should not get diagnostics for it.

Also see Microsoft C/C++ language conformance by Visual Studio version and P0386R2, Inline Variables.

irwir commented 1 year ago

Did static const achieve the goal of squashing the warning?

No, it did not.

Visual Studio 2017 support's C++17's inline variable.

Ability to build the library in old C++ compilers might be more valuable than a fix for this tiny issue.

noloader commented 1 year ago

@irwir ,

Does constexpr resolve the issue under Visual Studio? If so, we can probably use it instead.

We can use a define to switch between const and constexpr (from config_misc.h):

#if defined(CRYPTOPP_CXX11_CONSTEXPR)
#  define CRYPTOPP_STATIC_CONSTEXPR static constexpr
#  define CRYPTOPP_CONST_OR_CONSTEXPR constexpr
#  define CRYPTOPP_CONSTEXPR constexpr
#else
#  define CRYPTOPP_STATIC_CONSTEXPR static
#  define CRYPTOPP_CONST_OR_CONSTEXPR const
#  define CRYPTOPP_CONSTEXPR
#endif // CRYPTOPP_CXX11_CONSTEXPR

The change would look like:

diff --git a/config_misc.h b/config_misc.h
index 67b2d4ab..db358dcd 100644
--- a/config_misc.h
+++ b/config_misc.h
@@ -147,9 +147,11 @@
 // http://stackoverflow.com/a/13867690/608639
 #if defined(CRYPTOPP_CXX11_CONSTEXPR)
 #  define CRYPTOPP_STATIC_CONSTEXPR static constexpr
+#  define CRYPTOPP_CONST_OR_CONSTEXPR constexpr^M
 #  define CRYPTOPP_CONSTEXPR constexpr
 #else
 #  define CRYPTOPP_STATIC_CONSTEXPR static
+#  define CRYPTOPP_CONST_OR_CONSTEXPR const^M
 #  define CRYPTOPP_CONSTEXPR
 #endif // CRYPTOPP_CXX11_CONSTEXPR

diff --git a/cryptlib.h b/cryptlib.h
index 5d8792d6..adc0e9c0 100644
--- a/cryptlib.h
+++ b/cryptlib.h
@@ -127,7 +127,8 @@ enum CipherDir {
        DECRYPTION};

 /// \brief Represents infinite time
-const unsigned long INFINITE_TIME = ULONG_MAX;
+/// \sa https://github.com/weidai11/cryptopp/issues/1185^M
+CRYPTOPP_CONST_OR_CONSTEXPR unsigned long INFINITE_TIME = ULONG_MAX;^M

 // VC60 workaround: using enums as template parameters causes problems
 /// \brief Converts an enumeration to a type suitable for use as a template parameter
noloader commented 1 year ago

@irwir,

I cannot get this warning to trigger using Visual Studio 2022 from a Developer Prompt. The /w35264 drops C5264 down to warning level 3:

image

I also added this to cryptlib.h, with no joy:

#pragma warning(default:5264)

Can you please test the constexpr when you have a moment?


I am using cl.exe version 19.33.31630, which is Visual Studio 2022 version 17.3:

c:\Users\Jeff\Desktop\cryptopp-fork>cl.exe
Microsoft (R) C/C++ Optimizing Compiler Version 19.33.31630 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.
noloader commented 1 year ago

Ok, this was cleared via Pull Request #1186.

Good luck will all the Visual Studio warnings due to C5264.