Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1560/ for more details.

Where you touched lines, please, replace NULL by nullptr.

I compiled it, and it works for me. The patch looks good except NULLs.

In D642#27150, @vladislavbelov wrote:

Where you touched lines, please, replace NULL by nullptr.

Then we'd have to change even more lines to make the code consistent. I'd not do this as part of this patch, preferring nullptr for new code or when substantially changing something is fine, but the very few cases where using it over NULL makes an actual difference are very few.

I'm wondering if we should bump the map xml version, since an element that was previously handled now is an error. I guess I should look into doing something with that gaia player template work that's partially merged and would probably benefit from a bump too and then commit this and that and bump the version sometime after the release.

(Haven't tested it, but the code looks good.)

In D642#27163, @leper wrote:

In D642#27150, @vladislavbelov wrote:

Where you touched lines, please, replace NULL by nullptr.

Then we'd have to change even more lines to make the code consistent. I'd not do this as part of this patch, preferring nullptr for new code or when substantially changing something is fine, but the very few cases where using it over NULL makes an actual difference are very few.

Someone told me, that there is no reason to create a special patch for NULL updates.

Someone told me, that there is no reason to create a special patch for NULL updates.

Same as there is no real reason to replace NULL with nullptr just in a few places. And I'm not even sure if a full s/NULL/nullptr/g run on the whole codebase would even make sense. It seems like work just for the sake of doing something, regardless of how useful it actually is. If cleanup is needed to actually improve something (or it is so horrible that without cleaning doing any work would be hard) then cleanup should be done before fixing something. But patches that just change code for the sake of changing it without actually improving anything seem slightly pointless and don't really help a lot (see D90 for one example that is somewhat useful, but the amount of work spent on that doesn't correlate with the benefit it will bring).

In D642#27166, @leper wrote:

But patches that just change code for the sake of changing it without actually improving anything seem slightly pointless and don't really help a lot.

I agree with that. I'm mostly worrying about what may happen if someone will forget about NULLs (that it's a number and not a pointer).

(Also there is an unused CTerritoryManager member var (another non-existent class) in CWorld with a corresponding getter, a relic of the old simulation (before the big rewrite that replaced it with simulation2).)

In D642#27284, @Sandarac wrote:

(Also there is an unused CTerritoryManager member var (another non-existent class) in CWorld with a corresponding getter, a relic of the old simulation (before the big rewrite that replaced it with simulation2).)

Care to post another diff for that one?

For this diff I'd like a bump of the map version, apart from that it looks good (and more complete than some diff I had locally). I did somewhat finish up the patch about adding gaia to the player array in maps, so it might be nice to get these two in consecutively, so the map version bump is only done once and handles both of these.

In D642#28784, @leper wrote:

In D642#27284, @Sandarac wrote:

(Also there is an unused CTerritoryManager member var (another non-existent class) in CWorld with a corresponding getter, a relic of the old simulation (before the big rewrite that replaced it with simulation2).)

Care to post another diff for that one?

Okay, will do.

For this diff I'd like a bump of the map version, apart from that it looks good (and more complete than some diff I had locally). I did somewhat finish up the patch about adding gaia to the player array in maps, so it might be nice to get these two in consecutively, so the map version bump is only done once and handles both of these.

Okay, then every skirmish/scenario xml will need to be bumped too, of course, as part of this diff (like rP16478).

The Loader/LoaderThunks headers need to be kept in World.cpp for D684, so the diff should be updated anyways (it would be nice if someone could take a look at that diff too).

Okay, then every skirmish/scenario xml will need to be bumped too, of course, as part of this diff (like rP16478).

Got a change for that locally, currently part of the commit (I should check a few things and create a differential revision for that) for adding gaia to PlayerData. Also see P59, in case we'd like a script.

The Loader/LoaderThunks headers need to be kept in World.cpp for D684, so the diff should be updated anyways (it would be nice if someone could take a look at that diff too).

Ah, in that case include the change in this one and fix up the description a bit.

I think we should:

• make sure the loader/loeadthunk thing is still working (add them back? comments aren't 100% clear).
• remove the territory manager thing mentioned above.
• bump versions
• commit.

bump the years and rebase also I guess.