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

Codechange: use C++ containers for parsing the settings int lists #12436

Merged
merged 1 commit into from
Apr 20, 2024

Conversation

rubidium42
Copy link
Contributor

Motivation / Problem

In #12432 I used a hacky method to get rid of a bad lengthof and added a to-do. Now it's time to get rid of that to-do.

Description

In the past you passed ParseIntList a pointer and a number of elements. This works fine for C-style arrays or std::array, but it's cumbersome with std::vector as you need to preallocate memory in the hope that it's right.
Furthermore some other magic was performed to clamp values to be within a specific range. This could have unexpected side effects, for example 2**32-1 would not be within the bounds of signed long and clamped to 2**31-1 before later being converted back to an uint32.

Now ParseIntList returns an optional vector with uint32s. Each of the current user is either uint32 (NewGRF parameters, base graphics parameters, resolution) or uint8 (custom music playlist).

Also simplify the code a bit by:

  • using memset directly on the array, instead of memset on a temporary table and then copying values over.
  • removing arbitrary 64 element limit.
  • using saveload's WriteValue instead of our own version.

Finally the resolution's configuration suggests it's a signed integer, but it's stored in Dimension which uses uint. So, change the configuration to that as well.

Limitations

ParseIntList does not support all VarTypes, but given its limited use implementing support for that seems pointless. If it's needed at a later point, it shouldn't be too hard to add by going to parse a 64 bit integer. But out of scope for now.

I haven't put much thought into the best performance, as this is only used when reading the configuration file, so any gains would be really minimal over maintainability of the code..

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 touches english.txt or translations? Check the guidelines
  • 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, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@PeterN
Copy link
Member

PeterN commented Apr 6, 2024

I would prefer to remove the C-style array pointer and memset, etc...

Possible to pass a std::span and use std::fill to empty it.

@rubidium42
Copy link
Contributor Author

I would prefer to remove the C-style array pointer and memset, etc...

So would I! But that's way outside of the scope of this PR.
I do not think that the caller of LoadIntList should do a switch on VarType, cast the void* it got from GetVariableAddress to create the appropriate pointer type to then create a std::span so you can later std::fill it. That seems more way more convoluted than just using memset.

@PeterN
Copy link
Member

PeterN commented Apr 6, 2024

Oh it comes from that :(

@rubidium42 rubidium42 merged commit 1691b41 into OpenTTD:master Apr 20, 2024
14 checks passed
@rubidium42 rubidium42 deleted the vector-parse-intlist branch April 20, 2024 09:57
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