Added /Wall to possible warning levels for Visual Studio

[ethan-wallace]

This small PR follows an old discussion and closes #709

It applies only to VS2010 and above.

What does this PR do?

The current behaviour of the warnings keyword when using Premake for a Visual Studio project is as follows:

Level	Flag
Off	/W0
Default	/W3
Extra	/W4

As per the referenced issue, this PR changes the behaviour to the following (bringing it in-line with how these flags are generated when using GCC: and clang):

Level	Flag
Off	/W0
Default	/W3
High	/W4
Extra	/Wall

In src/tools/msc.lua the list of warning compiler flags contains both "High" and "Extra" with a value of "/W4", however in modules/vstudio/vs2010_vcxproj.lua the map to generate the XML Visual Studio expects only contains Off and Extra, so even if a user specified warnings "High" they would still get /W3 (the default when the passed value isn't in the map) instead of /W4 (which they probably expected). I downloaded a fresh Premake binary from the website and confirmed that this was indeed the case.

I have also altered the unit test in test_msc.lua, specifically suite.cflags_OnExtraWarnings, to ensure that the proper flag is generated. Running the Premake test suite I get no errors.

Testing was done on Windows 10 Pro (64-bit) version 1909.

How does this PR change Premake's behavior?

When running Premake with warnings "Extra" and generating a Visual Studio solution (VS2010 or above), the resultant .vcxproj will generate with <WarningLevel>EnableAllWarnings</WarningLevel> instead of <WarningLevel>Level4</WarningLevel>

Running Premake with warnings "High" and generating a Visual Studio solution (VS2010 or above), the resultant .vcxproj will generate with <WarningLevel>Level4</WarningLevel> instead of <WarningLevel>Level3</WarningLevel>

Anything else we should know?

The /Wall keyword includes all of the warnings in /W4 plus some extra ones off by default. These extra warnings can be turned on piecemeal via the enablewarnings keyword, but it would be nice to be able to just have to set the single flag.

In the referenced issue, a few commenters brought up the importance of allowing /W4 to remain available as an option, which this PR does (/W4 is still available through warnings "High").

Overall, I believe that this PR brings MSVC in line with how Premake deals with GCC; warnings "High" leads to the generation of the flag -Wall while warnings "Extra" leads to the generation of -Wall -Wextra.

[ethan-wallace]

Tests fail on CI, but that's to be expected as the XML output changed. Locally, when running bin/release/premake5 test I get all tests passed.

[starkos]

If you look at the CI test results you'll see that there are some additional unit tests that need to be patched up to reflect your change. Is that something you can take a look at?

Appreciate the detailed PR!

[ethan-wallace]

Oops, I see where the additional tests are now.... Will update them to reflect the new changes.

[samsinsane]

I'm not entirely sold on this PR. You're saying that -Wextra and /Wall are the same, but the linked issue says otherwise:

The msvc STL is only tested so that it doesn't emit /W4 errors. The visual c++ team generally recommends that you don't use /Wall all the time.

My projects use flags { "FatalWarnings" } with warnings "Extra", there's a few 3rd party files that we pull in that we opt to turn warnings off on entirely, but otherwise our entire project will build just fine with these two settings.

I appreciate that people want to turn /Wall on, but I don't think it's a great idea to complicate scripts to do:

warnings "Extra"
filter { "action:vs*" }
  warnings "High"
filter {}

just to achieve the same functionality that they have now with warnings "Extra".

[starkos]

I agree with the OP that High should be set to "4" for VS, which would bring it into alignment with the other toolsets, including MSC. The fact that High isn't mapped at all makes me think that no just ever got around to doing it. This may cause some existing projects to start generating more warnings, but I think we can make the case for that.

Having poked around a bit, I think we should leave Extra as it is at "4", see this SO thread for some explanation.

@ethan-wallace What say you? Would you be willing to rework this PR to just map High and leave Extra alone (and then squash it to remove the previous commits)? You can always add /Wall yourself for VS using buildoptions().

[redorav]

Would it make sense to have an All entry instead after Extra that maps to -Weverything? In case you really want to turn the entire world of warnings on?

[starkos]

Would it make sense to have an All entry instead after Extra that maps to -Weverything?

I'm good with that. Maybe we should call it "Everything"?

[ethan-wallace]

That sounds reasonable to me. If no-one has any further revisions I'll see what I can do about adding an "Everything" entry on top of "Extra". In this scenario, the new table would be as follows:

Level	Flag (MSVC)	Flag (GCC/Clang)
Off	/W0	-w
Default	/W3	no flag (gcc's default warnings)
High	/W4	-Wall
Extra	/W4	-Wall -Wextra
Everything	/Wall	-Weverything

I primarily use Visual Studio and MSVC, but looking at the code there doesn't seem to be a CFLAG emitted on GCC/clang for a default warning level. Let me know if I'm off about that. I also don't have much experience with the C# compilers - if needed I will go ahead with just MSVC and GCC, get your feedback, and then take a stab at dotnet just so everything is consistent across the toolsets.

[samsinsane]

The flags for GCC fall under the shared block, which is flags shared between C and C++.

As for what needs to be updated, anything that uses warnings should be updated:

• GCC:

  premake-core/src/tools/gcc.lua

  Lines 103 to 107 in b47a949

  	warnings = {
  	Extra = {"-Wall", "-Wextra"},
  	High = "-Wall",
  	Off = "-w",
  	},

• MSC:

  premake-core/src/tools/msc.lua

  Lines 82 to 86 in b47a949

  	warnings = {
  	Extra = "/W4",
  	High = "/W4",
  	Off = "/W0",
  	},

• SNC:

  premake-core/src/tools/snc.lua

  Lines 30 to 32 in b47a949

  	warnings = {
  	Extra = "-Xdiag=2",
  	}

• DMD:

  premake-core/modules/d/tools/dmd.lua

  Lines 300 to 304 in b47a949

  	warnings = {
  	Default = "-wi",
  	High = "-wi",
  	Extra = "-wi",
  	},

• GDC:

  premake-core/modules/d/tools/gdc.lua

  Lines 84 to 88 in b47a949

  	warnings = {
  	-- Default = "-w", -- TODO: check this...
  	High = "-Wall",
  	Extra = "-Wall -Wextra",
  	},

• LDC:

  premake-core/modules/d/tools/ldc.lua

  Lines 116 to 120 in b47a949

  	warnings = {
  	Default = "-wi",
  	High = "-wi",
  	Extra = "-wi", -- TODO: is there a way to get extra warnings?
  	},

• vcproj:

  premake-core/modules/vstudio/vs200x_vcproj.lua

  Lines 1591 to 1606 in b47a949

  	function m.warningLevel(cfg)
  	local prjcfg, filecfg = config.normalize(cfg)
  	local level
  	if cfg.warnings == p.OFF then
  	level = "0"
  	elseif cfg.warnings == "Extra" then
  	level = "4"
  	elseif not filecfg then
  	level = "3"
  	end
  	if level then
  	p.w('WarningLevel="%s"', level)
  	end
  	end

• vcxproj:

  premake-core/modules/vstudio/vs2010_vcxproj.lua

  Lines 2716 to 2719 in b47a949

  	function m.warningLevel(cfg)
  	local map = { Off = "TurnOffAllWarnings", Extra = "Level4" }
  	m.element("WarningLevel", nil, map[cfg.warnings] or "Level3")
  	end

• XCode:

  premake-core/modules/xcode/xcode_common.lua

  Lines 1451 to 1453 in b47a949

  	if cfg.warnings == "Extra" then
  	settings['WARNING_CFLAGS'] = '-Wall -Wextra'
  	end

Where you don't know what the value should be, use the value for Extra. Also, I can't see anything in the Clang docs that it supports -Weverything, so you may need to modify it to not copy the GCC warnings table, similar to floatingpoint or optimize above this:

premake-core/src/tools/clang.lua

Line 63 in b47a949

warnings = gcc.shared.warnings,

[starkos]

there doesn't seem to be a CFLAG emitted on GCC/clang for a default warning level

This is by design: Default leaves the toolset's default warning level unchanged, and is meant to be the equivalent of "add no flags".

[redorav]

Also, I can't see anything in the Clang docs that it supports -Weverything, so you may need to modify it to not copy the GCC warnings table, similar to floatingpoint or optimize above this:

Both Clang and GCC do support this flag (a quick godbolt experiment confirms it), what documentation did you check?

[samsinsane]

Both Clang and GCC do support this flag (a quick godbolt experiment confirms it)

I always forget that exists when it comes to flags, oops!

what documentation did you check?

Just the Clang docs, the flag doesn't seem to be listed, I might have missed it but searching the page for everything didn't return any results either.

[starkos]

Any thoughts or progress on this? I'm going to be pushing back a little harder on "stale" PRs so we don't end up with a collection of incomplete work that no one intends to finish (again). If this isn't be actively worked, let's create an issue to note it and then close the PR.

[ethan-wallace]

Most of the work has been done, have been obscenely busy the last few weeks with work. I'll see if I can get an update over the weekend. Sorry for the delay!

[starkos]

No worries! I certainly know how that is. Just checking in…

[ethan-wallace]

Finally had some time to get this squared away. I've added distinct "High", "Extra", and "Everything" flags as discussed, and where possible added tests for all of them (e.g., test_gcc.lua only had one test for warnings).

What I haven't added is the extra flags for the SNC compiler; I have no experience with it and couldn't find any documentation. I see there is a single flag in there right now for "Extra" but I didn't want to add any flags I wasn't 100% sure would actually be valid. If anyone has documentation/knows what flags are valid please let me know and I'll add them in. As well, I couldn't find documentation for what vcproj_200x based projects would emit on "Everything" (e.g., in vs2010+ the output would be EnableAllWarnings). I don't have a copy of VisualStudio that old laying around, so wasn't able to verify it. Again, if anyone knows what should go there please let me know.

Barring that, if everything looks good I'll squash the commits for merging

[starkos]

Looks good, thanks! Waiting the CI checks to pass…

[samsinsane]

Travis-CI seems incredibly flaky when it comes to connecting the PR to the build job. I've manually kicked it off, but one of our unit tests is failing due to httpbin.org having a broken redirect test.

@starkos I'm going to merge this since our master build is already failing for the same reason.

[haolly]

Please update the document https://premake.github.io/docs/warnings/ to reflect this change