This patch consists of three parts:
- 01-version-check.patch: Cleans up checking of savegame versions by introducing three functions: loadsave_t::is_version_atleast, loadsave_t::is_version_less and loadsave_t::is_version_equal. The first function can be used when introducing new data to the save game:
if (file->is_version_atleast(120, 8)) { /* load/save new data */ } The second function can be used for compatibility with old saves:
if (file->is_version_less(120, 8)) { /* code to load/save old saves */ }
This is IMO more readable and less error-prone than comparing the output of file->get_version() to a raw integer.
[/li]
- 02-cleanup-int-version.patch: Cleans up loadsave_t::int_version
- 03-fix-invalid-version-check.patch: Fixes a bug that caused invalid saves to be potentially recognized as valid saves
Quote from: ceeac on May 13, 2019, 09:30:46 AM01-version-check.patch: Cleans up checking of savegame versions by introducing three functions: loadsave_t::is_version_atleast, loadsave_t::is_version_less and loadsave_t::is_version_equal.
I have been considering introducing something similar to Extended (where I was thinking of names 'is_extended_version_before' and 'is_extended_version_after' to correspond to your first two functions). In Extended the current situation is more complicated than Standard because we're having to check for two different version numbers in combination (requiring three numerical comparisions) rather than encoding this in a single integer.
The old binary mode was different from the zipped mode. But such old games will fail anyway.
Why the test for "if (pak_extension_str) {"?
Also, why renaming "get_version" to "get_version_raw"? There is no gain I see, but making more conflict on patches.
Quote from: prissi on May 15, 2019, 12:14:14 AMWhy the test for "if (pak_extension_str) {"?
It may be NULL, see e.g. simmain.cc line 631. It is more of a precaution since currently for all calls to int_version where pak_extension_str is NULL, version_text contains a valid version string.
Quote from: prissi on May 15, 2019, 12:14:14 AMAlso, why renaming "get_version" to "get_version_raw"?
My intention was to make it clear that the function returns a raw integer as a version number and not a (hypothetical) version_t structure. But I can change it back if that's desired.
One could of course define version as an enum and generate warnings when compared to intergers.
But for your puspose, how about get_version_int? This makes much clearer that an integer is returned.
That is fine by me. I updated the patch files accordingly.
EDIT: Last patch file was broken, should be fixed now.
added in r8770 thanks