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 std::string exclusively for settings #9128

Merged
merged 11 commits into from May 13, 2021

Conversation

@rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented Apr 28, 2021

Motivation / Problem

Some (string) settings you want to validate before the setting is written as to prevent the user from entering something invalid. For example an empty client name. Now the (string) validation can only happen in the post set update, so there is no way to revert anymore. To implement that it would be nice not to have to worry about three types of strings coming from the settings: a C-string buffer, a heap allocated C-string and std::string.

For the media things a start has already been made with std::string, so this is just the continuation of that change.

Description

Everywhere where a C-string buffer or heap allocated C-string setting was used this is converted to std::string, along with some of the related logic for handling them. Arguably things might be pushed even further down the stacks, but that would balloon this PR so usually the bare minimum has been done.

Limitations

Empty digit separators are not possible anymore as they now fall back to the language's default.
The overall diff could be smaller if I chose to go back to SDT_STR etc, instead of SDT_SSTR. However, I chose not to do that so other people with patches get a compile time failure instead of a weird crash when it interprets char buffers as std::string or so.
Some variables, those that do not go over the network, I have generally not given an artificial limit as it's not really important how long e.g. a hostname is. It might now truncate it at a later stage in the application, but when the underlying code has been updated to fully use std::string that truncation problem should have gone away.

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')
src/settings.cpp Outdated Show resolved Hide resolved
src/network/network_client.cpp Outdated Show resolved Hide resolved
src/network/network_client.cpp Show resolved Hide resolved
src/string.cpp Outdated Show resolved Hide resolved
src/string.cpp Outdated Show resolved Hide resolved
src/string.cpp Outdated Show resolved Hide resolved
src/string.cpp Show resolved Hide resolved
src/newgrf.cpp Outdated Show resolved Hide resolved
src/newgrf.cpp Outdated Show resolved Hide resolved
src/newgrf.cpp Outdated Show resolved Hide resolved
@frosch123
Copy link
Member

@frosch123 frosch123 commented Apr 28, 2021

More general: You use std::string::length() everywhere.
Not sure whether we have decided for a style, but in my bubble people strongly prefer std::string::size().

  • It's consistent with std::vector.
  • Various 3rdparty unicode-aware string implementation use size() for the storage-size in bytes, and length() for the number of unicode-codepoints.

@rubidium42 rubidium42 force-pushed the string_settings branch 3 times, most recently from bd012d6 to e407e10 Apr 29, 2021
@rubidium42 rubidium42 marked this pull request as ready for review Apr 29, 2021
src/settings.cpp Outdated Show resolved Hide resolved
@rubidium42 rubidium42 force-pushed the string_settings branch 7 times, most recently from d2ce829 to 31b2713 May 6, 2021
@rubidium42 rubidium42 force-pushed the string_settings branch 2 times, most recently from b3f5485 to 24eda79 May 12, 2021
src/currency.h Outdated Show resolved Hide resolved
src/settings.cpp Outdated Show resolved Hide resolved
@rubidium42 rubidium42 merged commit 0f062b3 into OpenTTD:master May 13, 2021
13 checks passed
@rubidium42 rubidium42 deleted the string_settings branch May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants