Page MenuHomeWildfire Games
Differential D1396

Remove not needed checks and code for VS2015 [VS2013 -> VS2015]
ClosedPublic
Actions

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

Details

Reviewers
Itms
vladislavbelov
Commits
rP23416: Remove not needed checks and code for VS2015 [VS2013 -> VS2015]
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 5581
Build 9404: Vulcan BuildJenkins
Build 9403: arc lint + arc unit

Event Timeline

Silier 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

Harbormaster completed remote builds in B5581: Diff 6189.Mar 16 2018, 11:50 PM
Silier updated this revision to Diff 6597.May 20 2018, 2:00 PM
Silier edited the summary of this revision. (Show Details)
Silier 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
Vulcan added a comment.May 20 2018, 2:35 PM

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

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

Harbormaster completed remote builds in B6104: Diff 6597.May 20 2018, 2:35 PM
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
Silier 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.

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

1900 is msc version for VS2015

Vulcan added a comment.Jan 15 2019, 5:50 PM

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

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

Harbormaster completed remote builds in B6799: Diff 7351.Jan 15 2019, 5:50 PM
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
Silier removed a reviewer: Stan.May 3 2019, 10:16 AM
Silier added a comment.May 25 2019, 8:13 PM

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

Stan added a reviewer: vladislavbelov.Jun 20 2019, 3:43 PM
Silier added a comment.Jun 20 2019, 5:05 PM

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

vladislavbelov added a comment.Jun 26 2019, 9:32 PM
In D1396#83410, @Angen wrote:

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

Would you want to close?

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

-> Remove pragma warnings not needed for VS2015

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

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

Harbormaster completed remote builds in B8140: Diff 8672.Jul 1 2019, 9:36 AM
Silier added inline comments.Jul 1 2019, 9:47 AM
source/lib/precompiled.h
44 ↗(On Diff #8672)

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 ↗(On Diff #8672)

Maybe you could check the toolset version instead ?

Silier mentioned this in rP22511: Fix the -Wclass-memaccess compiler warning in the pathfinders Grid.h, refs….Jul 25 2019, 4:02 PM
Itms accepted this revision.Dec 30 2019, 6:35 PM

This is good for me! I think you can commit this just after I commit D2524.

It only needs a trivial rebase for LoaderThunks.h and I think you should rephrase the error message for the code that detects the minimal version of VS.

source/lib/precompiled.h
45 ↗(On Diff #8672)

I agree with this check, which will be useful and will be updated each time we drop support for a version.

I think the best formulation of the error would be Visual Studio 2015 is the minimal supported version.

This revision is now accepted and ready to land.Dec 30 2019, 6:35 PM
Silier planned changes to this revision.Jan 13 2020, 8:18 AM

needs rebase

Silier added a comment.Jan 16 2020, 8:31 AM
This comment was removed by Silier.
Stan added a comment.EditedJan 16 2020, 9:58 AM

Just for the record

_MSC_VER

source/collada/Decompose.cpp:#ifdef _MSC_VER
source/lib/external_libraries/suppress_boost_warnings.h:#if _MSC_VER > 1800
source/lib/sysdep/compiler.h:#ifdef _MSC_VER
source/lib/sysdep/compiler.h:# define MSC_VERSION _MSC_VER
source/lib/sysdep/os/win/wdbg_heap.h:#ifdef _MSC_VER
source/lib/sysdep/os/win/wdbg_heap.h:#if _MSC_VER > 1800
source/lib/sysdep/os/win/wdbg_sym.h:#ifdef _MSC_VER
source/lib/sysdep/os/win/wdbg_sym.h:#if _MSC_VER > 1800
source/lib/sysdep/os/win/wposix/wtime.h:#if _MSC_VER < 1900
source/lib/sysdep/os/win/wutil.h:#ifdef _MSC_VER
source/lib/sysdep/os/win/wutil.h:#if _MSC_VER > 1800
source/lib/sysdep/sysdep.h:#ifdef _MSC_VER
source/lib/sysdep/sysdep.h:#if _MSC_VER > 1800
source/third_party/cppformat/format.cpp:#ifndef _MSC_VER
source/third_party/cppformat/format.cpp:#else  // _MSC_VER
source/third_party/cppformat/format.cpp:#if _MSC_VER > 1800
source/third_party/cppformat/format.cpp:#endif  // _MSC_VER
source/third_party/cppformat/format.cpp:#ifdef _MSC_VER
source/third_party/cppformat/format.cpp:#ifdef _MSC_VER
source/third_party/cppformat/format.cpp:#ifdef _MSC_VER
source/third_party/cppformat/format.h:# if defined(_MSC_VER) && _MSC_VER >= 1800
source/third_party/cppformat/format.h:# elif defined(_MSC_VER) && _MSC_VER >= 1600
source/third_party/jsonspirit/json_spirit_error_position.h:#if defined(_MSC_VER) && (_MSC_VER >= 1020)
source/third_party/jsonspirit/json_spirit_reader_template.h:#if defined(_MSC_VER) && (_MSC_VER >= 1020)
source/third_party/jsonspirit/json_spirit_value.h:#if defined(_MSC_VER) && (_MSC_VER >= 1020)
source/third_party/jsonspirit/json_spirit_writer_options.h:#if defined(_MSC_VER) && (_MSC_VER >= 1020)
source/third_party/jsonspirit/json_spirit_writer_template.h:#if defined(_MSC_VER) && (_MSC_VER >= 1020)
source/third_party/mikktspace/mikktspace.cpp:#ifdef _MSC_VER
source/third_party/mikktspace/mikktspace.cpp:#if _MSC_VER > 1800
source/third_party/mikktspace/mikktspace.cpp:#ifdef _MSC_VER
source/third_party/mikktspace/weldmesh.cpp:#ifdef _MSC_VER
source/third_party/mikktspace/weldmesh.cpp:#if _MSC_VER > 1800
source/third_party/mongoose/mongoose.cpp:#ifdef _MSC_VER
source/third_party/mongoose/mongoose.cpp:#if _MSC_VER > 1800
source/third_party/mongoose/mongoose.cpp:#if defined(_MSC_VER) && _MSC_VER < 1300
source/third_party/mongoose/mongoose.cpp:#endif // _MSC_VER
source/third_party/mongoose/mongoose.cpp:#if _MSC_VER < 1900
source/third_party/mongoose/mongoose_orig.c:#if defined(_MSC_VER) && _MSC_VER < 1300
source/third_party/mongoose/mongoose_orig.c:#endif // _MSC_VER
source/third_party/tinygettext/src/unix_file_system.cpp:#ifdef _MSC_VER
source/tools/atlas/AtlasFrontends/_template.cpp:# if _MSC_VER >= 1400 // (can't be bothered to implement this for VC7.1...)
source/tools/atlas/AtlasObject/AtlasObjectImpl.h:#ifdef _MSC_VER
source/tools/atlas/AtlasObject/AtlasObjectJS.cpp:#if defined(_MSC_VER)
source/tools/atlas/AtlasObject/JSONSpiritInclude.h:#ifdef _MSC_VER
source/tools/atlas/AtlasObject/JSONSpiritInclude.h:#ifdef _MSC_VER
source/tools/atlas/AtlasUI/Misc/precompiled.h:#if defined(_MSC_VER)
source/tools/atlas/GameInterface/Messages.h:#ifdef _MSC_VER // (can't use MSC_VERSION here since this file is included by Atlas too)
source/tools/atlas/GameInterface/Messages.h:#ifdef _MSC_VER
source/tools/atlas/GameInterface/Shareable.h:#ifdef _MSC_VER // (can't use MSC_VERSION here since this file is included by Atlas too)
source/tools/atlas/GameInterface/Shareable.h:#ifdef _MSC_VER

MSC_VERSION

source/lib/alignment.h:#include "lib/sysdep/compiler.h" // MSC_VERSION
source/lib/alignment.h:#if MSC_VERSION
source/lib/byte_order.h:#elif MSC_VERSION
source/lib/code_annotation.h:#elif MSC_VERSION
source/lib/code_annotation.h:#if MSC_VERSION && !ICC_VERSION // (ICC ignores this)
source/lib/code_annotation.h:#elif MSC_VERSION
source/lib/code_annotation.h:# if MSC_VERSION
source/lib/code_annotation.h:#elif MSC_VERSION
source/lib/code_annotation.h:# error ICC_VERSION defined without either GCC_VERSION or an adequate MSC_VERSION
source/lib/code_annotation.h:#elif MSC_VERSION
source/lib/code_annotation.h:#if MSC_VERSION
source/lib/code_annotation.h:#if MSC_VERSION
source/lib/code_annotation.h:#if MSC_VERSION && !ARCH_AMD64
source/lib/debug.h:#if MSC_VERSION
source/lib/debug_stl.cpp:#if MSC_VERSION
source/lib/external_libraries/dbghelp.h:#if MSC_VERSION
source/lib/external_libraries/libsdl.h:# if MSC_VERSION
source/lib/external_libraries/openal.h:#if MSC_VERSION
source/lib/external_libraries/openmp.h:# if MSC_VERSION
source/lib/external_libraries/png.h:#if MSC_VERSION
source/lib/external_libraries/png.h:#endif      // MSC_VERSION
source/lib/external_libraries/powrprof.h:#if MSC_VERSION && MSC_VERSION < 1400
source/lib/external_libraries/powrprof.h:#endif // #if MSC_VERSION
source/lib/external_libraries/suppress_boost_warnings.h:#if MSC_VERSION
source/lib/external_libraries/tinygettext.h:#if MSC_VERSION
source/lib/external_libraries/tinygettext.h:#if MSC_VERSION
source/lib/external_libraries/vorbis.h:#if MSC_VERSION
source/lib/external_libraries/zlib.h:#if MSC_VERSION
source/lib/lib_api.h:# if MSC_VERSION
source/lib/ogl.cpp:#if MSC_VERSION
source/lib/pch/pch_boost.h:#if MSC_VERSION
source/lib/pch/pch_warnings.h:#include "lib/sysdep/compiler.h"  // MSC_VERSION
source/lib/pch/pch_warnings.h:#if MSC_VERSION
source/lib/pch/pch_warnings.h:#if MSC_VERSION <= 140
source/lib/pch/pch_warnings.h:#if MSC_VERSION <= 140
source/lib/posix/posix.h:#if MSC_VERSION
source/lib/posix/posix.h:#if MSC_VERSION
source/lib/posix/posix.h:# if MSC_VERSION
source/lib/precompiled.h:#include "lib/sysdep/compiler.h"    // MSC_VERSION
source/lib/precompiled.h:#if MSC_VERSION
source/lib/secure_crt.h:#if MSC_VERSION
source/lib/sysdep/acpi.cpp:#if MSC_VERSION
source/lib/sysdep/arch/amd64/amd64.cpp:#if MSC_VERSION
source/lib/sysdep/arch/ia32/ia32.cpp:#if MSC_VERSION
source/lib/sysdep/arch/x86_x64/x86_x64.cpp:#if MSC_VERSION
source/lib/sysdep/arch/x86_x64/x86_x64.cpp:#if !MSC_VERSION     // ensure not already defined in header
source/lib/sysdep/arch/x86_x64/x86_x64.cpp:#if MSC_VERSION
source/lib/sysdep/arch/x86_x64/x86_x64.h:#if MSC_VERSION
source/lib/sysdep/arch/x86_x64/x86_x64.h:#if MSC_VERSION
source/lib/sysdep/compiler.h:# define MSC_VERSION _MSC_VER
source/lib/sysdep/compiler.h:# define MSC_VERSION 0
source/lib/sysdep/compiler.h:# elif MSC_VERSION // also includes ICC
source/lib/sysdep/compiler.h:# elif MSC_VERSION // also includes ICC
source/lib/sysdep/cpu.h:#if MSC_VERSION && ARCH_X86_X64
source/lib/sysdep/os/win/manifest.cpp:#if MSC_VERSION >= 1400 && !ICC_VERSION && defined(LIB_STATIC_LINK)
source/lib/sysdep/os/win/wdbg_heap.cpp:#if MSC_VERSION && _DLL  // DLL runtime library
source/lib/sysdep/os/win/wdbg_heap.cpp:         const int dllVersion = (MSC_VERS
                // VC2005: 1400 => 80
source/lib/sysdep/os/win/wdll_delay_load.cpp:#if MSC_VERSION && MSC_VERSION >= 1700
source/lib/sysdep/os/win/wdll_ver.cpp:#if MSC_VERSION
source/lib/sysdep/os/win/wgfx.cpp:#if MSC_VERSION
source/lib/sysdep/os/win/whrt/hpet.cpp:#define HAVE_X64_MOVD ARCH_AMD64 && (ICC_VERSION || MSC_VERSION >= 1500)
source/lib/sysdep/os/win/whrt/tgt.cpp:#if MSC_VERSION
source/lib/sysdep/os/win/wposix/waio.cpp:#if MSC_VERSION
source/lib/sysdep/os/win/wposix/waio.cpp:#if MSC_VERSION
source/lib/sysdep/os/win/wposix/werrno.h:#if (!defined(BOOST_VERSION) || BOOST_VERSION <= 103401) && (!MSC_VERSION || MSC_VERSION < 1600)
source/lib/sysdep/os/win/wposix/wfilesystem.h:#if MSC_VERSION
source/lib/sysdep/os/win/wposix/wposix_types.h:#if MSC_VERSION
source/lib/sysdep/os/win/wposix/wposix_types.h:#if MSC_VERSION || ICC_VERSION || LCC_VERSION
source/lib/sysdep/os/win/wposix/wposix_types.h:#if MSC_VERSION || ICC_VERSION || LCC_VERSION
source/lib/sysdep/os/win/wposix/wposix_types.h:#if MSC_VERSION >= 1600
source/lib/sysdep/os/win/wseh.cpp:#if MSC_VERSION
source/lib/sysdep/os/win/wsysdep.cpp:#if MSC_VERSION
source/lib/sysdep/stl.h:#if MSC_VERSION
source/lib/tex/tex_png.cpp:#if MSC_VERSION
source/lib/tex/tex_png.cpp:#endif       // MSC_VERSION
source/main.cpp:#if MSC_VERSION
source/maths/Fixed.h:#if MSC_VERSION
source/pch/gui/precompiled.h:#if MSC_VERSION
source/ps/GameSetup/HWDetect.cpp:       scriptInterface.SetProperty(settings, "build_msc", (int)MSC_VERSION);
source/ps/LoaderThunks.h:#if MSC_VERSION
source/scriptinterface/ScriptExtraHeaders.h:#if MSC_VERSION
source/scriptinterface/ScriptExtraHeaders.h:#if MSC_VERSION
source/scriptinterface/ScriptTypes.h:#if MSC_VERSION
source/scriptinterface/ScriptTypes.h:#if MSC_VERSION
source/test_setup.cpp:#if MSC_VERSION
source/tools/atlas/GameInterface/Messages.h:#ifdef _MSC_VER // (can't use MSC_VERSION here since this file is included by Atlas too)
source/tools/atlas/GameInterface/Shareable.h:#ifdef _MSC_VER // (can't use MSC_VERSION here since this file is included by Atlas too)

Those are wrong

source/lib/pch/pch_warnings.h:#if MSC_VERSION <= 140
source/lib/pch/pch_warnings.h:#if MSC_VERSION <= 140
source/lib/precompiled.h
44 ↗(On Diff #8672)

So apparently no it is not possible to do so.

Silier added a comment.Jan 16 2020, 10:02 AM

Those are wrong
source/lib/pch/pch_warnings.h:#if MSC_VERSION <= 140
source/lib/pch/pch_warnings.h:#if MSC_VERSION <= 140

I know that was first intention in the diff :)

Stan added a comment.Jan 16 2020, 10:05 AM

I'm not sure about the change in Mongoose, there are quite a few warnings in that file. I think it will be better to update it.

Stan added a comment.Jan 16 2020, 10:47 AM

After giving it some thought I guess you can keep the changes in mongoose.

Silier updated this revision to Diff 11065.Jan 17 2020, 10:02 AM

rebase

This revision is now accepted and ready to land.Jan 17 2020, 10:02 AM
Vulcan added a comment.Jan 17 2020, 10:16 AM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1069/display/redirect

Vulcan added a comment.Jan 17 2020, 10:18 AM

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/165/display/redirect

Vulcan added a comment.Jan 17 2020, 10:38 AM

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

Linter detected issues:
Executing section Source...

source/lib/sysdep/os/win/wutil.h
|  37| template<typename·H>
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'template<...' is invalid C code. Use --std or --language to configure the language.

source/ps/LoaderThunks.h
|  28| template<class·T>·struct·MemFun_t
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'template<...' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1587/display/redirect

Harbormaster completed remote builds in B10843: Diff 11065.Jan 17 2020, 10:38 AM
Closed by commit rP23416: Remove not needed checks and code for VS2015 [VS2013 -> VS2015] (authored by Silier). · Explain WhyJan 19 2020, 1:44 PM
This revision was automatically updated to reflect the committed changes.
Silier added a commit: rP23416: Remove not needed checks and code for VS2015 [VS2013 -> VS2015].

Revision Contents
Changeset List

PathSize
source/
lib/
adts/
6 lines
file/
io/
4 lines

Diff 6189

View Options

source/lib/adts/ring_buf.h

Loading...
View Options

source/lib/file/io/io.h

Loading...
Wildfire Games · Phabricator