As reported by Angen and historic_bruno in #5539, the default initialization of array is not implemented correctly in VS2013 (but VS2015), so rP22511 broke there.
This patch uses the previous memset as we didn't find a less broken implementation than that yet (PathCost ctor is still triggering a warning and it's one Ctor modification away from breaking).
Details
Details
Verify that this introduces the code as it was before. Test whether keeping the correct lines in the repository is the right choice as opposed to removing them (as the code isn't broken but VS2013 is) and that VS2013 will be dropped in the next weeks by looks of things (#5379) which would mean that the C++11 / GCC compliant code can be used again without removing and reintroducing the lines (but only reintroducing and removing the broken lines temporarily).
Figure out whether there is a way to init that array without breaking on VS2013, without triggering a GCC compiler warning, without skipping the PathCost ctor.
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 8602 Build 14087: Vulcan Build Jenkins Build 14086: arc lint + arc unit
Event Timeline
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/helpers/Grid.h | 37| template<typename·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/236/display/redirect
Comment Actions
Here's the observed behavior in VS2013:
- pre-rP22511: builds and can play matches (tested last week when I was still using VS2013).
- rP22511: builds, but instant crashes on match start (I didn't tested this as I had switched to VS2015... was not reported either). Also introduced memleak.
- This is important because we hadn't introduced placement-new to fix the memleak. That's evidence that it can be ruled out as the problem. More later.
- rP22545: builds, fixes memleak, but crashes on match start (I also didn't test this on VS2013... but gameboy and Angen did, and reported the crash).
- Suspicion falls on placement-new, as one might expect, it was all that changed in this commit.
- I tested after the report from Angen and confirmed the crash. Also suspected placement-new, but while testing things, found that rP22511 crashes too.
So it must be something in rP22511. Three candidates by process of elimination, basically.
- The following diff builds, but crashes:
Index: Grid.h =================================================================== --- Grid.h (revision 22551) +++ Grid.h (working copy) @@ -217,8 +217,7 @@ for (size_t i = 0; i < (size_t)(m_BW*m_BH); ++i) delete[] m_Data[i]; - // Reset m_Data by value-constructing in place with placement new. - m_Data = new (m_Data) T*[m_BW*m_BH](); + memset(m_Data, 0, m_BW*m_BH*sizeof(T*)); } void set(int i, int j, const T& value)
- Next, this diff also builds but crashes:
Index: Grid.h =================================================================== --- Grid.h (revision 22551) +++ Grid.h (working copy) @@ -203,7 +203,8 @@ m_BW = (u16)((m_W + BucketSize-1) >> BucketBits); m_BH = (u16)((m_H + BucketSize-1) >> BucketBits); - m_Data = new T*[m_BW*m_BH](); + m_Data = new T*[m_BW*m_BH]; + memset(m_Data, 0, m_BW*m_BH*sizeof(T*)); } ~SparseGrid() @@ -217,8 +218,7 @@ for (size_t i = 0; i < (size_t)(m_BW*m_BH); ++i) delete[] m_Data[i]; - // Reset m_Data by value-constructing in place with placement new. - m_Data = new (m_Data) T*[m_BW*m_BH](); + memset(m_Data, 0, m_BW*m_BH*sizeof(T*)); } void set(int i, int j, const T& value)
The only changes left from rP22511 are:
Index: Grid.h =================================================================== --- Grid.h (revision 22510) +++ Grid.h (revision 22511) @@ -1,4 +1,4 @@ -/* Copyright (C) 2018 Wildfire Games. +/* Copyright (C) 2019 Wildfire Games. * This file is part of 0 A.D. * * 0 A.D. is free software: you can redistribute it and/or modify @@ -190,8 +190,7 @@ size_t b = (j >> BucketBits) * m_BW + (i >> BucketBits); if (!m_Data[b]) { - m_Data[b] = new T[BucketSize*BucketSize]; - memset(m_Data[b], 0, BucketSize*BucketSize*sizeof(T)); + m_Data[b] = new T[BucketSize*BucketSize](); } return m_Data[b]; }
That's why I said it must be something about array initialization and not placement-new, it's the only thing left.
| source/simulation2/helpers/Grid.h | ||
|---|---|---|
| 30 | you could use here visual studio version here to set this value | |
Comment Actions
The diff is actually unsatisfying because it's still triggering the compile warning and there is the possibility that VS2013 is not dropped in the next few weeks.
| source/simulation2/helpers/Grid.h | ||
|---|---|---|
| 30 | I don't know if one can, especially if one could what bag of weirdness one imports | |
Comment Actions
We could replace the memset with a loop, though that's somewhat likely to be slower as it might not be optimised well.
TBH I'd be tempted to just call it a day for VS13 :/
What's the reason for wanting to keep it right now?
This comment was removed by wraitii.
Comment Actions
If VS2013 actually would be dropped in 1-2 weeks, I actually don't see the point of replacing it either.
I guess it wasn't 1-2 weeks, but I still don't see the point in saving something that is scheduled to be dropped.
| source/simulation2/helpers/Grid.h | ||
|---|---|---|
| 30 | MSC_VERSION from compiler.h looks nicer, but I'm not convinced that it's a good idea to write platform dependent compiler code, hence this fallback is 1 for all compilers. | |