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

build: Disable Visual Studio warning "conditional expression is const… #4658

Closed
wants to merge 2 commits into from

Conversation

jay
Copy link
Member

@jay jay commented Nov 30, 2019

…ant"

  • Disable warning C4127 "conditional expression is constant" when
    building with Microsoft's compiler.

C4127 was already disabled in the limited circumstance of the WHILE_FALSE
macro which disabled the warning specifically for while(0). This commit
removes the WHILE_FALSE macro in favor of disabling C4127 everywhere.

We have various macros that cause 0 or 1 to be evaluated, which causes
warning C4127 in Visual Studio. For example this causes it:

#define Curl_resolver_asynch() 1

Full behavior is not clearly defined and inconsistent across versions.
However it is documented that since VS 2015 Update 3 Microsoft has
addressed this somewhat but not entirely, not warning on while(true) for
example.

Prior to this change some C4127 warnings occurred when I built with
Visual Studio.

Closes #xxxx


/cc @danielgustafsson

…ant"

- Disable warning C4127 "conditional expression is constant" when
  building with Microsoft's compiler.

C4127 was already disabled in the limited circumstance of the WHILE_FALSE
macro which disabled the warning specifically for while(0). This commit
removes the WHILE_FALSE macro in favor of disabling C4127 everywhere.

We have various macros that cause 0 or 1 to be evaluated, which causes
warning C4127 in Visual Studio. For example this causes it:

#define Curl_resolver_asynch() 1

Full behavior is not clearly defined and inconsistent across versions.
However it is documented that since VS 2015 Update 3 Microsoft has
addressed this somewhat but not entirely, not warning on while(true) for
example.

Prior to this change some C4127 warnings occurred when I built with
Visual Studio.

Closes #xxxx
@jay jay added build Windows Windows-specific labels Nov 30, 2019
@danielgustafsson
Copy link
Member

I don't know Visual Studio so I might be off, but won't this risk that code which should've invoked an actual warning now passes?

@jay
Copy link
Member Author

jay commented Nov 30, 2019

I don't know Visual Studio so I might be off, but won't this risk that code which should've invoked an actual warning now passes?

Yeah but that's the point. In my opinion the warning is a nuisance and only VS does this. I can't recall ever seeing a conditional constant expression that actually was a mistake.

@MarcelRaad
Copy link
Member

I've actually seen C4127 discover a bug once. IIRC, it was a comparison which should have been an assignment and the variable was wrongly "fixed" to be const instead of assigned to. Nothing that should ever have passed code review though, so I'm fine with disabling it.

One thing though, it's disabled for the CMake build in

set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /wd4127")
. That could then be removed.

4127 is disabled globally in curl_setup.h which is as close as we get so
it no longer needs to be disabled in each individual build system.
@jay
Copy link
Member Author

jay commented Nov 30, 2019

One thing though, it's disabled for the CMake build in

Thanks. I didn't realize that prior to this change 4127 was already disabled in the other build systems that can use microsoft's compiler, winbuild and cmake. Since this commit adds a single disable in curl_setup.h that should work for all build systems. I've now removed the others (see the squashme).

@danielgustafsson
Copy link
Member

danielgustafsson commented Nov 30, 2019 via email

@jay jay closed this in 9c1806a Dec 2, 2019
@jay jay deleted the disable_vs_warning_4127_take2 branch December 2, 2019 00:03
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

None yet

3 participants