search

SergiusTheBest / FindWDK

CMake module for building drivers with Windows Development Kit (WDK)
BSD 3-Clause "New" or "Revised" License
251 stars 53 forks source link

"Fixed" Command line warning D9025 : overriding '/W3' with '/W4' #1

Closed sgeto closed 6 years ago

sgeto commented 6 years ago

Hi

/W3 is part of CMake's default compiler flags. Adding /W4 the way you did won't overwrite it. All samples are being built with /W3 instead of /W4 and /WX. Appveyor will break with this PR now, because of the two warnings that are now treated as errors (/W4 and /WX are now working)

C:\FindWDK\samples\WdmDriver\Main.c(5): warning C4100: 'driverObject': unreferenced formal parameter
C:\FindWDK\samples\WdmDriver\Main.c(11): warning C4100: 'registryPath': unreferenced formal parameter

Your module has the same issue with /GR vs. /GR- The former is the default and must be removed first. The module should probably start things off with something similar than my foreach(WARN_FLAG) loop to remove all default flags before adding new ones.

SergiusTheBest commented 6 years ago

Hi,

Thanks for the pull request!

It's interesting how did you get warning D9025? My compiler doesn't say anything bad. I am expecting that the latest options override previous ones and it's a normal and documented behaviour. But now I'm puzzled that the warning D9025 exists.

Indeed the warning level wasn't set for the C compiler. I've added set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /W4 /WX") and fixed the warnings.

sgeto commented 6 years ago

It's interesting how did you get warning D9025? My compiler doesn't say anything bad.

Change your Generator to NMake Makefiles and define -DCMAKE_VERBOSE_MAKEFILE=1 to see all flags and warnings. MSBuild isn't capable of that. 👍

I've added set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /W4 /WX") and fixed the warnings.

I would advise you to

  1. Make your module handle this (not CMakeLists.txt) since this file is only used by your project. Users will face this problem again.

  2. Why not overwriting/clearing all default flags before adding yours? In my opinion defining -DWIN32 or -DWINDOWS can cause problems when building a driver. And there's also the issue with /GR vs. /GR-

I really like this module. I can easily imagine it to become the de facto standard. Keep it up!

SergiusTheBest commented 6 years ago

The idea is not to force users to /W4 /WX and let them choose. Also replacing directly in CMAKE_C_FLAGS will affect all other projects. But the approach is good. I'll take a look how to limit it only to WDK projects.

sgeto commented 6 years ago

Thanks. I'll get back to you if there's anything else.