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

Assorted C-string -> std::string conversions in network #9371

Merged
merged 9 commits into from Jun 15, 2021

Conversation

@rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented Jun 13, 2021

Motivation / Problem

The ongoing conversion of C-string to std::string.

Description

All kinds of small, but scattered in networking code, changes to go from C-strings to std::string.

  • Use fmt in the network address creation.
  • Use std::string is IsInNetmask; https://godbolt.org/z/n8TTM5osz is a simple sort of unit test for this change.
  • Let GetHostName/GetClientIP directly return the std::string reference instead of the C-string, to prevent converting is back to a std::string later on (e.g. in IsInNetmask), but also in other locations.
  • Let NetworkError just return a reference to its std::string.
  • Use fmt in HTTP request creation and pass the host as std::string reference.
  • Use std::string_view for network compatability checks; https://godbolt.org/z/sj9Ya3q6W is a simple unit test for this change.
  • Use std::string for returning the client name.
  • Use std::string for (temporarily) storing the name of the downloading content.

Limitations

Contains #9372.

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')
@rubidium42 rubidium42 force-pushed the more_string_in_network branch from a25fb05 to a9acc07 Jun 13, 2021
@rubidium42 rubidium42 marked this pull request as ready for review Jun 14, 2021
src/network/core/tcp_http.cpp Show resolved Hide resolved
Loading
src/network/network_server.cpp Outdated Show resolved Hide resolved
Loading
@rubidium42 rubidium42 force-pushed the more_string_in_network branch from cd86685 to 4be0d8a Jun 14, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

YOLO!

Loading

Copy link
Member

@TrueBrain TrueBrain left a comment

Sure!

Loading

@@ -1272,7 +1272,7 @@ static void AILoadConfig(IniFile *ini, const char *grpname)
continue;
}
}
if (item->value.has_value()) config->StringToSettings(item->value->c_str());
if (item->value.has_value()) config->StringToSettings(*item->value);
Copy link
Contributor Author

@rubidium42 rubidium42 Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather use item->value.value() but... https://twitter.com/timur_audio/status/1184424350105161730?lang=nl

Loading

@rubidium42 rubidium42 merged commit d31a535 into OpenTTD:master Jun 15, 2021
15 checks passed
Loading
@rubidium42 rubidium42 deleted the more_string_in_network branch Jun 15, 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

2 participants