Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch saveload versions from literal numbers to enum values. #7148

Merged
merged 4 commits into from Feb 2, 2019

Conversation

@PeterN
Copy link
Member

PeterN commented Jan 30, 2019

This PR switches saveload versions from literal numbers to enum values. While this doesn't really change much in the saveload system, nor OpenTTD itself, it benefits patch maintainers in that merging savegame bumps will be simpler due to being in a single (conflicting) location instead of spread through code which often does not conflict at all.

Currently literal numbers are replaced with an equivalent SLV_num, but future additions should use more descriptive names. Current SLV_num entries can also be changed very simply with an in-place regex over the complete source.

@PeterN PeterN force-pushed the PeterN:saveload-version-enum branch 2 times, most recently from 81ccdee to 7e876b9 Jan 31, 2019
@PeterN

This comment has been minimized.

Copy link
Member Author

PeterN commented Feb 1, 2019

Rebased to due to recent savegame bump, ironically :-)

SLV_200, // 200 PR#6805 Extend railtypes to 64, adding uint16 to map array.
SLV_201, // 201 PR#6885 Extend NewGRF persistant storages.
SLV_202, // 202 PR#6867 Increase industry cargo slots to 16 in, 16 out
SLV_203, // 203 PR#7072 Add path cache for ships

This comment has been minimized.

Copy link
@J0anJosep

J0anJosep Feb 1, 2019

Contributor

Is it possible to give last saveloads a more descriptive name than SLV_203? Let's say SLV_203_SHIP_CACHE?
It may be helpful to have them as an example for future naming of saveload versions.

This comment has been minimized.

Copy link
@PeterN

PeterN Feb 1, 2019

Author Member

Yes, the intention is to not include the number, however, as that would kinda defeat the point. I had started it but then a rogue regex wiped everything out and I scrapped it :-)

This comment has been minimized.

Copy link
@J0anJosep

J0anJosep Feb 1, 2019

Contributor

Just last ones, to make clear future developers how to give proper names to saveload versions.

This comment has been minimized.

Copy link
@J0anJosep

J0anJosep Feb 1, 2019

Contributor

I would keep saveload number in the name for merged commits.

This comment has been minimized.

Copy link
@PeterN

PeterN Feb 1, 2019

Author Member

Hmm, that may end up with future additions following suit, when it's unnecessary and creates more work.

This comment has been minimized.

Copy link
@J0anJosep

J0anJosep Feb 1, 2019

Contributor

The numbers on the saveload names may be helpful if we find a bug on an old savegame. Knowing which pieces of code are executed and which ones aren't on AfterLoad() is important.

This comment has been minimized.

Copy link
@Eddi-z

Eddi-z Feb 2, 2019

Contributor

I disagree, the number should not be part of (future) saveload enum entries, because the whole point of the exercise is to make it easier to maintain patches with savegame bumps, who do not know yet what the final number will be.

@PeterN

This comment has been minimized.

Copy link
Member Author

PeterN commented Feb 1, 2019

The magic incantation to bulk rename is:
find -type f | xargs perl -pi -e 's/SLV_num/SLV_descriptive_name/g'

@J0anJosep

This comment has been minimized.

Copy link
Contributor

J0anJosep commented Feb 1, 2019

What is the benefit of making saveload version upper bound exclusive?

Can't we just start making use of the enum since version 200? Changing so many numbers will create lots of conflicts on previously written code.

@PeterN

This comment has been minimized.

Copy link
Member Author

PeterN commented Feb 1, 2019

As an example, if it was still inclusive, then entry in company_sl.cpp couldn't use SLV_EXTEND_CARGOTYPES as the upper bound:

        SLE_CONDARR(CompanyEconomyEntry, delivered_cargo,     SLE_UINT32, 32,           SLV_170, SLV_EXTEND_CARGOTYPES),
        SLE_CONDARR(CompanyEconomyEntry, delivered_cargo,     SLE_UINT32, NUM_CARGO,    SLV_EXTEND_CARGOTYPES, SL_MAX_VERSION),
@J0anJosep

This comment has been minimized.

Copy link
Contributor

J0anJosep commented Feb 1, 2019

As an example, if it was still inclusive, then entry in company_sl.cpp couldn't use SLV_EXTEND_CARGOTYPES as the upper bound:

        SLE_CONDARR(CompanyEconomyEntry, delivered_cargo,     SLE_UINT32, NUM_CARGO,    SLV_EXTEND_CARGOTYPES, SL_MAX_VERSION),```

Well, would these work?

	SLE_CONDARR(CompanyEconomyEntry, delivered_cargo,     SLE_UINT32, 32,           170, SLV_EXTEND_CARGOTYPES - 1),
	SLE_CONDARR(CompanyEconomyEntry, delivered_cargo,     SLE_UINT32, NUM_CARGO,    SLV_EXTEND_CARGOTYPES, SL_MAX_VERSION),

It is what we have now.

@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented Feb 1, 2019

Exclusive upper bound seems clearer to me.

@PeterN

This comment has been minimized.

Copy link
Member Author

PeterN commented Feb 1, 2019

Needs a cast to compile with the -1. That could be put in the macro but then it loses type-safety.

@PeterN PeterN force-pushed the PeterN:saveload-version-enum branch from 4dbf3e6 to c4ee40a Feb 2, 2019
@PeterN

This comment has been minimized.

Copy link
Member Author

PeterN commented Feb 2, 2019

Now using doxygen comments for each entry.

@J0anJosep

This comment has been minimized.

Copy link
Contributor

J0anJosep commented Feb 2, 2019

Thanks for your answers. Now I see why these changes are necessary.

@michicc
michicc approved these changes Feb 2, 2019
@PeterN PeterN merged commit e21ade3 into OpenTTD:master Feb 2, 2019
1 check passed
1 check passed
OpenTTD CI Build #20190202.1 succeeded
Details
@PeterN PeterN deleted the PeterN:saveload-version-enum branch Feb 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.