cmake: Silent MSVC warnings #44
Conversation
hebasto
force-pushed
the
231105-cmake-U
branch
from
9678941
to
39f12b7
Compare
CMakeLists.txt
Outdated
|
|
||
| # Production quality warning level. | ||
| try_append_cxx_flags("/W3" TARGET core) | ||
| # Disable warning C4018 "'token' : signed/unsigned mismatch". |
There was a problem hiding this comment.
Do we really need to copy-paste the definitions of all these MSVC warning flags in here?
There was a problem hiding this comment.
At least, it does not hurt. It's also a nice way (at least for me) to document the silenced warnings, providing a quick understanding of their severity. In this PR, I followed the same practice that was accepted in the secp256k1 repo.
Anyway, I'm open to alternative approaches.
There was a problem hiding this comment.
It introduces a lot of verbosity, for something that anyone can easily lookup? I don't think we should do this specially for MSVC, if we aren't doing it for all other warning / compile / link flags?
hebasto
force-pushed
the
231105-cmake-U
branch
from
39f12b7
to
816fd3b
Compare
| if(MSVC) | ||
| target_compile_options(leveldb | ||
| PRIVATE | ||
| /wd4244 |
There was a problem hiding this comment.
Does it make sense to set these if they are set anyway in the top level CMakeLists.txt?
There was a problem hiding this comment.
At some point in the future, we might want to modify our codebase to fix warnings. It is unlikely that we want to touch subtree codebase for the same reason.
There was a problem hiding this comment.
lgtm ACK 816fd3b
Tested commit-by-commit on msvc. The small cleanups and improvements compared with the current msvc definitions also look good to me.
This PR follows the master branch approach and amends the previous commits.
Subtrees are treated separately from our code.
Also please refer to:
bitcoin/build_msvc/libleveldb/libleveldb.vcxproj
Line 54 in 953d302
bitcoin/build_msvc/libminisketch/libminisketch.vcxproj
Line 31 in 953d302
bitcoin/build_msvc/libsecp256k1/libsecp256k1.vcxproj
Line 20 in 953d302
bitcoin/build_msvc/common.init.vcxproj.in
Lines 91 to 93 in 953d302
and build: Drop no longer needed MSVC warning suppressions bitcoin/bitcoin#28798
All warning suppressions are documented,