I have

From bc47dc4fa901206b25d648f1b819abeccc4c3646 Mon Sep 17 00:00:00 2001
From: leper <leper@wildfiregames.com>
Date: Fri, 10 Nov 2017 10:32:36 +0100
Subject: [PATCH 1/2] Explicitly initialize members that are read from before
 being set.

This seems to fix a test failure I encountered when compiling with clang.
---
 source/simulation2/helpers/Grid.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/source/simulation2/helpers/Grid.h b/source/simulation2/helpers/Grid.h
index 37baf51a01..97d5e610b2 100644
--- a/source/simulation2/helpers/Grid.h
+++ b/source/simulation2/helpers/Grid.h
@@ -46,9 +46,8 @@ public:
 		reset();
 	}
 
-	Grid(const Grid& g)
+	Grid(const Grid& g) : m_W(0), m_H(0), m_Data(NULL)
 	{
-		m_Data = NULL;
 		*this = g;
 	}
 
-- 
2.15.0

locally, which as the commit message states fixed some test failure on clang. It seems that test failure actually showed up in-game on some systems.

My best guess is that Clang assumes, since m_W and m_H aren't initialized, that the checks we do in the copy operator are UB and it can assume they're equal or something. Value-initializing them is cleaner and fixes the issue.

Likely. I also considered that (since I only had that in a single test, and there were a few tests with the same grid sizes) the new grid ending up in the same place on the stack, thus the sizes being the same, and correct, the memcpy not doing anything since passing null to it would be undefined and the compiler just inlined things there.

The checks being undefined is not the case IIRC. Implementation-defined or unspecified likely, but well we are possibly reading random garbage that might just be whatever we want. I do agree that actually initializing them is the right thing to do, especially since we actually read from two of them before doing anything.

Edit: also is this C++11 only? I don't remember

Assuming that refers to using = 123 for members, IIRC yes. Then again I do consider that ugly.

Also I somewhat prefer my change, mostly because it is shorter.

(In case it wasn't obvious I didn't actually check if my guess was correct, in case someone is bored enough to just run the specific test and that still fails, then it wasn't.)

I personally don't agree with the = 123 syntax being ugly, and it seems to be the recommended way to do things in modern C++: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-in-class-initializer

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content

The recommended way also lists an alternative, though that one has some caveat. For something like the listed example I do agree with it being preferable, for a big codebase that did get some C++11 updates where needed adding a few such features is not that nice, especially since one would not expect them to be there. Since a quick grep of the codebase grep "^\s[A-Za-z0-9_]\+\sm_" source -r | grep = and some manual weeding out of the few results we notice that there exist a total of two such occurences in the current code base, both being introduced by rP19031...

My issue with using that in this file is that it just makes the codebase inconsistent, and it mostly violates the part about at least keeping things consistent locally, and that part is not a neccessary change here. (Though even a diff for the whole codebase to make use of that would be quite unlikely to get anyone interested in it.)

Mh, I do feel like in-class initializers are infinitely preferable to constructor initialisers, and VS 2013 does support it.
It would undeniably make this file inconsistent however, so I suppose it'd be better to actually change most of the codebase in a separate diff (or at the least most of the code in simulation2/