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
Conversation
81ccdee
to
7e876b9
Compare
Rebased to due to recent savegame bump, ironically :-) |
src/saveload/saveload.h
Outdated
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
The magic incantation to bulk rename is: |
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. |
As an example, if it was still inclusive, then entry in company_sl.cpp couldn't use SLV_EXTEND_CARGOTYPES as the upper bound:
|
Well, would these work?
It is what we have now. |
Exclusive upper bound seems clearer to me. |
Needs a cast to compile with the -1. That could be put in the macro but then it loses type-safety. |
… object was removed instead of version object last appeared.
(This was mostly achieved with a few in-place regexes)
4dbf3e6
to
c4ee40a
Compare
Now using doxygen comments for each entry. |
Thanks for your answers. Now I see why these changes are necessary. |
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.