Fix #8713: Change OTTD2FS and FS2OTTD to return string objects #8716
Conversation
nielsmh
force-pushed
the
fix-8713
branch
3 times, most recently
from
e8eda83 to
a9dcb52
Compare
|
Redid the UNICODE removal in #8720 instead. |
| @@ -279,7 +279,7 @@ bool FioCheckFileExists(const std::string &filename, Subdirectory subdir) | |||
| */ | |||
| bool FileExists(const std::string &filename) | |||
| { | |||
| return access(OTTD2FS(filename.c_str()), 0) == 0; | |||
| return access(OTTD2FS(filename).c_str(), 0) == 0; | |||
There was a problem hiding this comment.
I'm concerned about the correctness of OTTD2FS(..).c_str() and it's use of the temporary std::string object. Is that definitely all correct?
There was a problem hiding this comment.
In this case the temporary will not be destructed until the end of the statement (after access has returned), so it is OK.
There was a problem hiding this comment.
I went over the changes and checked, yes all the cases of OTTD2FS(x).c_str() are safe: The C string returned is only expected to live until the called function has returned. All the cases where it caused problems the temporary string returned from the conversion function is stored into a proper local variable to manage its lifetime.
Motivation / Problem
The
OTTD2FSandFS2OTTDfunctions are not thread safe, they use static (global) buffers to hold the returned string data and require the caller to copy the data to somewhere better, and pray there isn't any race condition.Description
Change the functions to return
std::stringandstd::wstringobjects instead, to get automatic lifetime management of the data, and avoid data sharing between threads.Additionally, this removes mostly any pretense that we can still build Windows 95 versions of the game.
Limitations
Several of these changes are done blind, so far. Only tested in a single configuration on Windows, the changes for other OS may not compile.
There are unrelated changes here that should probably be split off.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.
This PR affects the save game format? (label 'savegame upgrade')This PR affects the GS/AI API? (label 'needs review: Script API')This PR affects the NewGRF API? (label 'needs review: NewGRF')