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 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 saveload-version-enum branch 2 times, most recently from 81ccdee to 7e876b9 Compare February 1, 2019 18:31
@PeterN
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@PeterN PeterN Feb 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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
Copy link
Contributor

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
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
Copy link
Contributor

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
Copy link
Contributor

glx22 commented Feb 1, 2019

Exclusive upper bound seems clearer to me.

@PeterN
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
Copy link
Member Author

PeterN commented Feb 2, 2019

Now using doxygen comments for each entry.

@J0anJosep
Copy link
Contributor

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

@PeterN PeterN merged commit e21ade3 into OpenTTD:master Feb 2, 2019
@PeterN PeterN deleted the saveload-version-enum branch February 2, 2019 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants