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 marked this pull request as ready for review June 14, 2021 14:19
src/network/core/tcp_http.cpp Show resolved Hide resolved
src/network/network_server.cpp Outdated Show resolved Hide resolved
TrueBrain
TrueBrain previously approved these changes Jun 14, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

YOLO!

Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

Sure!

@@ -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

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

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

2 participants