Page MenuHomeWildfire Games

Remove not needed checks and code for VS2015 [VS2013 -> VS2015]
Needs ReviewPublic

Authored by Angen on Mar 16 2018, 8:47 PM.

Details

Summary

Some pragma warnings are not more needed for VS2015 compilation this will remove them.
Removing some specific code for older version than VS2015.
Forcing build to fail if compiling with VS older than VS2015.

Test Plan

Check we did not enable not wanted warning by accident.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8140
Build 13255: Vulcan BuildJenkins
Build 13254: arc lint + arc unit

Event Timeline

Angen created this revision.Mar 16 2018, 8:47 PM
Vulcan added a subscriber: Vulcan.Mar 16 2018, 11:49 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/259/display/redirect

Angen updated this revision to Diff 6597.May 20 2018, 2:00 PM
Angen edited the summary of this revision. (Show Details)
Angen edited the test plan for this revision. (Show Details)

In D1262 have been added version check for warning C4351, which was not correct and because of that, warning have been enabled again for VS2013. I have no idea how VS2015 managed to not display this warning since version did not satisfy needed version check to disable this warning.

Stan accepted this revision.May 20 2018, 2:20 PM
Stan added a reviewer: Itms.
This revision is now accepted and ready to land.May 20 2018, 2:20 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/529/display/redirect

Itms requested changes to this revision.Dec 30 2018, 5:37 PM

Not sure either why VS2015 doesn't get the error. Could you try to see what happens when the version check is removed? It doesn't make sense to add this new conditional as-is. The only meaningful patches would be, In my opinion, no conditional (the warning is silenced on all versions), or a conditional that keeps the old behavior for 2013 and introduces a change for the new version (that would be <= 1800).

Also, the typo was made twice in this file, for warning C4836 as well.

I'm removing the Accepted status while this is investigated.

This revision now requires changes to proceed.Dec 30 2018, 5:37 PM
Angen added a comment.Jan 1 2019, 8:05 PM

So a bit of research and test builds as well.
https://docs.microsoft.com/en-us/previous-versions/1ywe7hcy(v=vs.140)

Looks like VS2013 uses older compiler for which is 4351 dangerous and can lead to unexpected behaviour, but for VS2015 it is safe by compiler.

One situation where the new behavior can result in unexpected behavior is when placement new is used to construct the object that has the array as a member, and the program depends on the contents that the memory (for the elements of the default initialized array) had before the call to placement new. In this case, the older compiler would have left the contents of memory unchanged, but the new behavior will cause default initialization of the array elements, overwriting the original contents in memory.

and

In previous versions of Visual C++, when an array was in a constructor's member initialization list, the elements of the array may not have been default initialized in some cases.

So to the builds without version guard.
VS2013,
not disabled c4351 -> a bunch of warnings
disabled c4351 -> no warning related to this
VS2015
not disabled c4351 -> no warning related to this
not disabled c4351 and building with Wall option -> no warning related to this

Looks like VS2015 stopped worried about 4351 warning so it now depends how long there will be game support for 2013. After that one drops, pragma warning for that can be removed.

Angen updated this revision to Diff 7351.Jan 15 2019, 5:25 PM

1900 is msc version for VS2015

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/973/

Stan added a comment.Feb 8 2019, 9:44 AM

For future reference.

https://dev.to/yumetodo/list-of-mscver-and-mscfullver-8nd

Still don't know why but we have our own macro in source/lib/sysdep/compiler.h :

#ifdef _MSC_VER
# define MSC_VERSION _MSC_VER
#else
# define MSC_VERSION 0
#endif
Angen removed a reviewer: Stan.May 3 2019, 10:16 AM
Angen added a comment.May 25 2019, 8:13 PM

@Itms ? could we have it ? VS13 builds are really noisy

Angen added a comment.Jun 20 2019, 5:05 PM

as we are now compiling game with vs15, this is not needed

In D1396#83410, @Angen wrote:

as we are now compiling game with vs15, this is not needed

Would you want to close?

Angen planned changes to this revision.Jun 30 2019, 8:26 PM

-> Remove pragma warnings not needed for VS2015

Angen retitled this revision from Silence C4351 to Remove not needed pragma warning for VS2015 [VS2013 -> VS2015].Jun 30 2019, 8:27 PM
Angen edited the summary of this revision. (Show Details)
Angen edited the test plan for this revision. (Show Details)
Angen updated this revision to Diff 8672.Jul 1 2019, 9:19 AM
Angen retitled this revision from Remove not needed pragma warning for VS2015 [VS2013 -> VS2015] to Remove not needed checks and code for VS2015 [VS2013 -> VS2015].
Angen edited the summary of this revision. (Show Details)

Removes checks introduced because we supported vs2013.
Removes dead code because of vs2015.
Makes build fail if compiling with older version than VS2015.

Stan added a subscriber: Stan.Jul 1 2019, 9:35 AM
Stan added inline comments.
source/lib/precompiled.h
44

might want to check != Vs2015 in case someone tries to build with something like vs2012 etc.

Vulcan added a comment.Jul 1 2019, 9:36 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lib/sysdep/os/win/wposix/wtime.h
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1863/display/redirect

Angen added inline comments.Jul 1 2019, 9:47 AM
source/lib/precompiled.h
44

vs2012 is 1700 so still works, and I actually cant exclude > 1900 because VS2017

Stan added inline comments.Jul 1 2019, 9:54 AM
source/lib/precompiled.h
44

Maybe you could check the toolset version instead ?