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

Several code refactors of the SaveLoad code #9338

Merged
merged 6 commits into from Jun 10, 2021

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 6, 2021

Depends on #9335

Motivation / Problem

When you start fiddling with the SaveLoad code, you find many odd things. And sometimes they annoy me enough that you get this collection of commits.

Description

First commit is #9335, after that:

  • First patch is some code hygiene
  • Second patch too
  • Third: SL_DEQEUE suggested to support signed values, but they were silently cast to unsigned .. which, with conversion code like SLE_FILE_I8 | SLE_VAR_I32 might not work as you expect. Luckily, nobody was using this code for signed values, but let's make sure we are ahead of this issue.
  • Forth: SlList is very similar to SlDeque, but a copied the code. So, together with the third commit, I changed the SlDequeHelper into a more general one, where anything supporting begin() and end() will work on.
  • Fifth: SL_LST is a list of references, which the name did not tell you. The way reference are done is already very error-prone, and this wasn't helping. So I renamed it to SL_REFLST to be more obvious of its function.
  • Sixth: cheat-chunk was weird. Just that. Weird. Made it slightly less weird.

I could have made 4 PRs out of this too, so if this is too hard to review, let me know, and I will split it up :)

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain TrueBrain force-pushed the saveload-refactor branch 2 times, most recently from a403080 to 4446f55 Compare June 6, 2021 10:26
@TrueBrain TrueBrain force-pushed the saveload-refactor branch 4 times, most recently from 19b9a72 to 7f0bcdd Compare June 7, 2021 09:55
src/saveload/saveload.cpp Outdated Show resolved Hide resolved
src/saveload/saveload.h Outdated Show resolved Hide resolved
src/saveload/saveload.cpp Outdated Show resolved Hide resolved
src/saveload/saveload.cpp Outdated Show resolved Hide resolved
src/saveload/saveload.cpp Outdated Show resolved Hide resolved
src/saveload/saveload.cpp Show resolved Hide resolved
src/saveload/saveload.h Outdated Show resolved Hide resolved
@TrueBrain TrueBrain force-pushed the saveload-refactor branch 3 times, most recently from 46ae39a to b669beb Compare June 7, 2021 22:10
src/saveload/saveload.cpp Outdated Show resolved Hide resolved
src/saveload/saveload.cpp Outdated Show resolved Hide resolved
Also move it to static, as nobody else is using it.
…e generic

Future additions will start using it for std::list too.
…part

You can easily mistake SlList / SL_LST to be a list of SL_VAR, but
it is a list of SL_REF. With this rename, it hopefully saves a few
people from "wtf?" moments.
@TrueBrain TrueBrain merged commit 1749524 into OpenTTD:master Jun 10, 2021
@TrueBrain TrueBrain deleted the saveload-refactor branch June 10, 2021 17:18
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

3 participants