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

winbuild: build with warning level 4 #1667

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@MarcelRaad
Member

MarcelRaad commented Jul 8, 2017

This is consistent with 7bc6456, which
changed the warning level from 3 to 4 for the Visual Studio project
files. Also define WIN32_LEAN_AND_MEAN consistently, which avoids
unnecessarily including stuff that emits a lot of level 4 warnings when
using older Windows SDK versions.

@mention-bot

This comment has been minimized.

mention-bot commented Jul 8, 2017

@MarcelRaad, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yangtse, @mback2k and @msnelling to be potential reviewers.

@coveralls

This comment has been minimized.

coveralls commented Jul 8, 2017

Coverage Status

Coverage increased (+0.06%) to 76.029% when pulling 4899662 on MarcelRaad:winbuild_w4 into a548183 on curl:master.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 9, 2017

"AppVeyor was unable to build non-mergeable pull request" - no idea what that means and found little info on the internet. I hope that will go away after rebase and force-push.

@MarcelRaad MarcelRaad force-pushed the MarcelRaad:winbuild_w4 branch from 4899662 to 19c0336 Jul 9, 2017

MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Jul 9, 2017

winbuild: build with warning level 4
This is consistent with 7bc6456, which
changed the warning level from 3 to 4 for the Visual Studio project
files. Also define WIN32_LEAN_AND_MEAN consistently, which avoids
unnecessarily including stuff that emits a lot of level 4 warnings when
using older Windows SDK versions.

Closes curl#1667
@coveralls

This comment has been minimized.

coveralls commented Jul 9, 2017

Coverage Status

Coverage decreased (-0.008%) to 75.953% when pulling 19c0336 on MarcelRaad:winbuild_w4 into 59a0fb2 on curl:master.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 9, 2017

Original problem disappeared, now it's

fatal: early EOF
fatal: The remote end hung up unexpectedly
fatal: index-pack failed
error: RPC failed; curl 18 transfer closed with outstanding read data remaining
Command exited with code 128

for one of the AppVeyor builds :-(

@jay

This comment has been minimized.

Member

jay commented Jul 9, 2017

Give it some time, that seems to occasionally with CI builds, they are overloaded and things slow down to the point where nothing can happen.

I tried this in both VS 2008 and 2010 and I get one real warning which I just fixed and then a bunch of superfluous C4127: conditional expression is constant.

+..\lib\cookie.c(921) : warning C4127: conditional expression is constant
+..\lib\cookie.c(1076) : warning C4127: conditional expression is constant
+..\lib\cookie.c(1077) : warning C4127: conditional expression is constant
+..\lib\cookie.c(1078) : warning C4127: conditional expression is constant
+..\lib\cookie.c(1079) : warning C4127: conditional expression is constant
+..\lib\cookie.c(1080) : warning C4127: conditional expression is constant
+..\lib\cookie.c(1081) : warning C4127: conditional expression is constant
+..\lib\cookie.c(1082) : warning C4127: conditional expression is constant
+..\lib\cookie.c(1083) : warning C4127: conditional expression is constant
+..\lib\url.c(1068) : warning C4127: conditional expression is constant
+..\lib\url.c(3575) : warning C4127: conditional expression is constant
+..\lib\multi.c(943) : warning C4127: conditional expression is constant
+..\lib\multi.c(944) : warning C4127: conditional expression is constant
+..\lib\multi.c(947) : warning C4127: conditional expression is constant
+..\lib\multi.c(948) : warning C4127: conditional expression is constant
+..\lib\select.c(270) : warning C4127: conditional expression is constant
+..\lib\select.c(271) : warning C4127: conditional expression is constant
+..\lib\select.c(276) : warning C4127: conditional expression is constant
+..\lib\select.c(277) : warning C4127: conditional expression is constant
+..\lib\select.c(285) : warning C4127: conditional expression is constant
+..\lib\select.c(286) : warning C4127: conditional expression is constant
+..\lib\select.c(484) : warning C4127: conditional expression is constant
+..\lib\select.c(486) : warning C4127: conditional expression is constant
+..\lib\select.c(488) : warning C4127: conditional expression is constant

So I'm sure those are all while(0 or 1 etc. Depending on how easy it is we could do them all with _pragmas using a macro like WHILE_FALSE or for while(1) use for(;;) since iirc vs doesn't warn for that. The select though is already a macro FD_SET which is defined in winsock and uses while(0), that might not be possible to wrap, older vs is picky about when it applies some __pragmas

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 9, 2017

Ah, I forgot about that warning :-( I tested with VS 2015 and 2017 with Windows SDK 10, which replaces that warning in the FD_SET macro with C4548 (expression before comma has no effect), which is off by default.

Now I remember that C4127 is also issued with certain libcurl feature combinations because some functions become macros defined to a constant. I'm not sure if it will be easy to catch and change all of them. Maybe we could just suppress it by building with /wd4127, and also for the VS project files? I always compile with

/Wall /WX /wd4127 /wd4255 /wd4548 /wd4668 /wd4710 /wd4711 /wd4774 /wd4820

myself instead of the default /W4 when using the VS project files, 4127 being the only warning which is on by default.

@bagder

This comment has been minimized.

Member

bagder commented Jul 10, 2017

It'd be cool to also make the cmake build generate that warning level, as that would then make the appveyor CI builds run like that and better help us maintain this level!

winbuild: build with warning level 4
This is consistent with 7bc6456, which
changed the warning level from 3 to 4 for the Visual Studio project
files. Also define WIN32_LEAN_AND_MEAN consistently, which avoids
unnecessarily including stuff that emits a lot of level 4 warnings when
using older Windows SDK versions.

Closes #1667

@MarcelRaad MarcelRaad force-pushed the MarcelRaad:winbuild_w4 branch from 19c0336 to 87e64ac Jul 11, 2017

@coveralls

This comment has been minimized.

coveralls commented Jul 11, 2017

Coverage Status

Coverage decreased (-0.004%) to 75.948% when pulling 87e64ac on MarcelRaad:winbuild_w4 into beb0848 on curl:master.

@bagder

bagder approved these changes Jul 13, 2017

@bagder bagder added the build label Jul 13, 2017

@MarcelRaad MarcelRaad deleted the MarcelRaad:winbuild_w4 branch Jul 13, 2017

@jay

This comment has been minimized.

Member

jay commented Jul 14, 2017

Maybe we could just suppress it by building with /wd4127, and also for the VS project files?

I think that should be fine but to gather any objections I've taken it to the mailing list first.
https://curl.haxx.se/mail/lib-2017-07/0042.html

@jay

This comment has been minimized.

Member

jay commented Jul 14, 2017

Maybe we could just suppress it by building with /wd4127, and also for the VS project files?

ah I see you've already disabled it in winbuild, I'd leave it for now and we'll see if anyone objects on the mailing list. If not I'll add it to the project files as well.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 14, 2017

I think that should be fine but to gather any objections I've taken it to the mailing list first.
https://curl.haxx.se/mail/lib-2017-07/0042.html

Thank you! I have to keep in mind the mailing list more.

MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Aug 3, 2017

CMake: set MSVC warning level to 4
The MSVC warning level defaults to 3 in CMake. Change it to 4, which is
consistent with the Visual Studio and NMake builds. Disable level 4
warning C4127 for the library and additionally C4306 for the test
servers to get a clean CURL_WERROR build as that warning is raised in
some macros in older Visual Studio versions.

Ref: curl#1667 (comment)
Closes curl#1711
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment