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

winbuild: build with warning level 4 #1667

Closed
wants to merge 1 commit into from

Conversation

MarcelRaad
Copy link
Member

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
Copy link

@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
Copy link

Coverage Status

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

@MarcelRaad
Copy link
Member Author

"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 added a commit to MarcelRaad/curl that referenced this pull request Jul 9, 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.

Closes curl#1667
@coveralls
Copy link

Coverage Status

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

@MarcelRaad
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

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
Copy link
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!

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
Copy link

Coverage Status

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

@bagder bagder added the build label Jul 13, 2017
@MarcelRaad MarcelRaad deleted the winbuild_w4 branch July 13, 2017 21:04
@jay
Copy link
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
Copy link
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
Copy link
Member Author

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
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
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants