Validate that "max" values in settings fit in their storage container #8780
Conversation
| def = DEF_SNOWLINE_HEIGHT * TILE_HEIGHT | ||
| min = MIN_SNOWLINE_HEIGHT * TILE_HEIGHT | ||
| max = MAX_SNOWLINE_HEIGHT * TILE_HEIGHT | ||
| # "max" used to be MAX_SNOWLINE_HEIGHT * TILE_HEIGHT, but this would overflow the storage. | ||
| max = UINT8_MAX |
There was a problem hiding this comment.
Wait a minute here, what unit is snow line height measured in? Because as far as I remember, TILE_HEIGHT is a constant for converting height levels into pixel lengths. Are sub-tile height snow line heights supported even?
There was a problem hiding this comment.
Don't get me started about this mess. But there is a AfterLoad routine that converts the values here back to their non-TILE_HEIGHT counter-part. So in our current code, snow_line_height is just a number between 1 and 255. Just for older savegames it can have other values ...
I wasn't sure how to fix this properly btw, so I went with this. "dead" code anyway, was savegame revision 22 .. lol :)
https://github.com/OpenTTD/OpenTTD/blob/master/src/saveload/afterload.cpp#L2774
There was a problem hiding this comment.
To add to that, I have no clue when clamping is done .. before or after afterload. As there is a second snow_line_height entry, that is the current version, but the afterload suggests older versions can have the value multiplied by TILE_HEIGHT too .. so if it is clamped before afterload, that line in afterload won't work anyway ..
This confuses me :P
Cleaning up mistakes of the past :D
…ored So any old game made with this setting was overflowing anyway; not really a lot we can do about that now.
TrueBrain
force-pushed
the
validate-max-fits-in-storage
branch
3 times, most recently
from
3509bc4 to
2539652
Compare
This is an easy mistake to make, so protect us against making such mistakes, by validating it doesn't happen.
TrueBrain
force-pushed
the
validate-max-fits-in-storage
branch
from
2539652 to
8f57c3a
Compare
Fixes #8610.
Motivation / Problem
Yesterday I added a setting where the maximum value exceeded the storage size. I was not aware of this, and nothing told me it was wrong. The reviewer missed it too.
To prevent this simple mistake from ever happening again, we should simply validate this.
Found 4 current instances that already were out-of-bounds. By accident this fixes an issue :D
Description
Make
settingsgengenerate a post-amble that does a ton ofstatic_assertto validatemaxis within the storage size assigned to the setting. This means that if you add a new setting, the compiler will tell you you suck at cookie, euh, coding.It required moving some code in
settingsgento its own function, to avoid code duplication.This could not be done by
settingsgenbtw, assettingsgendoesn't understand the values it is reading/writing .. it just copies them byte-by-byte. This makes sense, as you can use constants, additions, etc etc.Fixed the four issues found too.
Limitations
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.