Skip to content

Commit

Permalink
Use fs::path for paths instead of std::string or char[MAX_PATH].
Browse files Browse the repository at this point in the history
  • Loading branch information
@Jarod42
Jarod42 committed Aug 4, 2023
1 parent f6be62c commit 8cd05ad
Show file tree
Hide file tree
Showing 22 changed files with 205 additions and 417 deletions.
1 change: 0 additions & 1 deletion CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,6 @@ set(stratagus_generic_HDRS
src/include/game.h
src/include/icons.h
src/include/interface.h
src/include/iocompat.h
src/include/iolib.h
src/include/luacallback.h
src/include/map.h
Expand Down
24 changes: 8 additions & 16 deletions src/action/actions.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@
#include "commands.h"
#include "game.h"
#include "interface.h"
#include "iocompat.h"
#include "luacallback.h"
#include "map.h"
#include "missile.h"
Expand Down Expand Up @@ -486,26 +485,19 @@ static void DumpUnitInfo(CUnit &unit)
time_t now;
time(&now);

std::string path(Parameters::Instance.GetUserDirectory());
fs::path path(Parameters::Instance.GetUserDirectory());
if (!GameName.empty()) {
path += "/";
path += GameName;
}
path += "/logs";
struct stat tmp;
if (stat(path.c_str(), &tmp) < 0) {
makedir(path.c_str(), 0777);
path /= GameName;
}
path /= "logs";
fs::create_directories(path);

path += "/unit_log_of_stratagus_";
path += std::to_string(ThisPlayer->Index);
path += "_";
path += std::to_string((intmax_t)now);
path += ".log";
path /= "unit_log_of_stratagus_" + std::to_string(ThisPlayer->Index) + "_"
+ std::to_string((intmax_t) now) + ".log";

logf = fopen(path.c_str(), "wb");
logf = fopen(path.string().c_str(), "wb");
if (!logf) {
DebugPrint("Could not open %s for writing\n" _C_ path.c_str());
DebugPrint("Could not open %s for writing\n" _C_ path.string().c_str());
return ;
}
fprintf(logf, "; Log file generated by Stratagus Version " VERSION "\n");
Expand Down
7 changes: 3 additions & 4 deletions src/editor/editloop.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
#include "game.h"
#include "guichan.h"
#include "interface.h"
#include "iocompat.h"
#include "iolib.h"
#include "map.h"
#include "menus.h"
Expand Down Expand Up @@ -1882,15 +1881,15 @@ static void EditorCallbackExit()
void CEditor::Init()
{
// Load and evaluate the editor configuration file
const std::string filename = LibraryFileName(Parameters::Instance.luaEditorStartFilename.c_str());
if (access(filename.c_str(), R_OK)) {
const fs::path filename = LibraryFileName(Parameters::Instance.luaEditorStartFilename.c_str());
if (!fs::exists(filename)) {
fprintf(stderr, "Editor configuration file '%s' was not found\n"
"Specify another with '-E file.lua'\n",
Parameters::Instance.luaEditorStartFilename.c_str());
ExitFatal(-1);
}

ShowLoadProgress(_("Script %s"), filename.c_str());
ShowLoadProgress(_("Script %s"), filename.string().c_str());
LuaLoadFile(filename);
LuaGarbageCollect();

Expand Down
89 changes: 30 additions & 59 deletions src/game/game.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
#include "editor.h"
#include "font.h"
#include "interface.h"
#include "iocompat.h"
#include "iolib.h"
#include "map.h"
#include "minimap.h"
Expand Down Expand Up @@ -182,47 +181,19 @@ void FreeAllContainers()
*/
static void LoadStratagusMap(const std::string &smpname, const std::string &mapname)
{
char mapfull[PATH_MAX];
CFile file;

fs::path mapfull = mapname;
// Try the same directory as the smp file first
strcpy_s(mapfull, sizeof(mapfull), smpname.c_str());
char *p = strrchr(mapfull, '/');
if (!p) {
p = mapfull;
} else {
++p;
}
strcpy_s(p, sizeof(mapfull) - (p - mapfull), mapname.c_str());

if (file.open(mapfull, CL_OPEN_READ) == -1) {
// Not found, try StratagusLibPath and the smp's dir
strcpy_s(mapfull, sizeof(mapfull), StratagusLibPath.c_str());
strcat_s(mapfull, sizeof(mapfull), "/");
strcat_s(mapfull, sizeof(mapfull), smpname.c_str());
char *p = strrchr(mapfull, '/');
if (!p) {
p = mapfull;
} else {
++p;
}
strcpy_s(p, sizeof(mapfull) - (p - mapfull), mapname.c_str());
if (file.open(mapfull, CL_OPEN_READ) == -1) {
// Not found again, try StratagusLibPath
strcpy_s(mapfull, sizeof(mapfull), StratagusLibPath.c_str());
strcat_s(mapfull, sizeof(mapfull), "/");
strcat_s(mapfull, sizeof(mapfull), mapname.c_str());
if (file.open(mapfull, CL_OPEN_READ) == -1) {
// Not found, try mapname by itself as a last resort
strcpy_s(mapfull, sizeof(mapfull), mapname.c_str());
} else {
file.close();
}
} else {
file.close();
// Not found, try StratagusLibPath and the smp's dir
// Not found again, try StratagusLibPath
// Not found, try mapname by itself as a last resort
for (auto candidate : {fs::path(smpname).parent_path() / mapname,
fs::path(StratagusLibPath) / fs::path(smpname).parent_path() / mapname,
fs::path(StratagusLibPath) / mapname,
fs::path(mapname)}) {
if (fs::exists(candidate)) {
mapfull = candidate;
break;
}
} else {
file.close();
}

if (LcmPreventRecurse) {
Expand All @@ -232,7 +203,7 @@ static void LoadStratagusMap(const std::string &smpname, const std::string &mapn
InitPlayers();
LcmPreventRecurse = 1;
if (LuaLoadFile(mapfull) == -1) {
fprintf(stderr, "Can't load lua file: %s\n", mapfull);
fprintf(stderr, "Can't load lua file: %s\n", mapfull.string().c_str());
ExitFatal(-1);
}
LcmPreventRecurse = 0;
Expand All @@ -252,7 +223,7 @@ static void LoadStratagusMap(const std::string &smpname, const std::string &mapn
}

// Write a small image of map preview
static void WriteMapPreview(const char *mapname, CMap &map)
static void WriteMapPreview(const fs::path &mapname, CMap &map)
{
const int rectSize = 5; // size of rectange used for player start spots
const SDL_PixelFormat *fmt = MinimapSurface->format;
Expand All @@ -273,7 +244,7 @@ static void WriteMapPreview(const char *mapname, CMap &map)
}

SDL_UnlockSurface(preview);
IMG_SavePNG(preview, mapname);
IMG_SavePNG(preview, mapname.string().c_str());
SDL_FreeSurface(preview);
}

Expand Down Expand Up @@ -345,12 +316,12 @@ static int WriteMapPresentation(const std::string &mapname, CMap &map, Vec2i new
** @param map map to save
** @param writeTerrain write the tiles map in the .sms
*/
int WriteMapSetup(const char *mapSetup, CMap &map, int writeTerrain, Vec2i newSize, Vec2i offset)
static int WriteMapSetup(const fs::path &mapSetup, CMap &map, int writeTerrain, Vec2i newSize, Vec2i offset)
{
FileWriter *f = nullptr;

try {
f = CreateFileWriter(mapSetup);
f = CreateFileWriter(mapSetup.string());

f->printf("-- Stratagus Map Setup\n");
f->printf("-- File generated by the Stratagus V" VERSION " builtin map editor.\n");
Expand All @@ -360,7 +331,7 @@ int WriteMapSetup(const char *mapSetup, CMap &map, int writeTerrain, Vec2i newSi
f->printf("-- preamble\n");
f->printf("if CanAccessFile(__file__ .. \".preamble\") then Load(__file__ .. \".preamble\", Editor.Running == 0) end\n\n");
if (!Map.Info.Preamble.empty()) {
FileWriter *preamble = CreateFileWriter(std::string(mapSetup) + ".preamble");
FileWriter *preamble = CreateFileWriter(mapSetup.string() + ".preamble");
preamble->write(Map.Info.Preamble.c_str(), Map.Info.Preamble.size());
delete preamble;
}
Expand Down Expand Up @@ -516,13 +487,13 @@ int WriteMapSetup(const char *mapSetup, CMap &map, int writeTerrain, Vec2i newSi
f->printf("-- postamble\n");
f->printf("if CanAccessFile(__file__ .. \".postamble\") then Load(__file__ .. \".postamble\", Editor.Running == 0) end\n\n");
if (!Map.Info.Postamble.empty()) {
FileWriter *postamble = CreateFileWriter(std::string(mapSetup) + ".postamble");
FileWriter *postamble = CreateFileWriter(mapSetup.string() + ".postamble");
postamble->write(Map.Info.Postamble.c_str(), Map.Info.Postamble.size());
delete postamble;
}

} catch (const FileException &) {
fprintf(stderr, "Can't save map setup : '%s' \n", mapSetup);
fprintf(stderr, "Can't save map setup : '%s' \n", mapSetup.string().c_str());
delete f;
return -1;
}
Expand All @@ -547,25 +518,26 @@ int SaveStratagusMap(const std::string &mapName, CMap &map, int writeTerrain, Ve
ExitFatal(-1);
}

char mapSetup[PATH_MAX];
strcpy_s(mapSetup, sizeof(mapSetup), mapName.c_str());
char *setupExtension = strstr(mapSetup, ".smp");
if (!setupExtension) {
std::string extraExtension; // typically compression extension
fs::path mapSetup = mapName;
while (mapSetup.has_extension() && mapSetup.extension() != ".smp") {
extraExtension = mapSetup.extension().string() + extraExtension;
mapSetup.replace_extension();
}
if (mapSetup.extension() != ".smp") {
fprintf(stderr, "%s: invalid Stratagus map filename\n", mapName.c_str());
return -1;
}

char previewName[PATH_MAX];
strcpy_s(previewName, sizeof(previewName), mapName.c_str());
char *previewExtension = strstr(previewName, ".smp");
memcpy(previewExtension, ".png\0", 5 * sizeof(char));
fs::path previewName = mapSetup;
previewName.replace_extension(".png");
WriteMapPreview(previewName, map);

memcpy(setupExtension, ".sms", 4 * sizeof(char));
if (WriteMapPresentation(mapName, map, newSize) == -1) {
return -1;
}

mapSetup.replace_extension(".sms" + extraExtension);
return WriteMapSetup(mapSetup, map, writeTerrain, newSize, offset);
}

Expand Down Expand Up @@ -1166,8 +1138,7 @@ static int CclSetGameName(lua_State *l)
}

if (!GameName.empty()) {
std::string path = Parameters::Instance.GetUserDirectory() + "/" + GameName;
makedir(path.c_str(), 0777);
fs::create_directories(Parameters::Instance.GetUserDirectory() / GameName);
}
return 0;
}
Expand Down
31 changes: 11 additions & 20 deletions src/game/replay.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
#include "commands.h"
#include "game.h"
#include "interface.h"
#include "iocompat.h"
#include "iolib.h"
#include "map.h"
#include "netconnect.h"
Expand Down Expand Up @@ -371,34 +370,27 @@ void CommandLog(const char *action, const CUnit *unit, int flush,
if (!LogFile) {
time_t now;
time(&now);
struct stat tmp;

std::string path(Parameters::Instance.GetUserDirectory());
fs::path path(Parameters::Instance.GetUserDirectory());
if (!GameName.empty()) {
path += "/";
path += GameName;
path /= GameName;
}
path += "/logs";
path /= "logs";

if (stat(path.c_str(), &tmp) < 0) {
makedir(path.c_str(), 0777);
}
fs::create_directories(path);

path += "/log_of_stratagus_";
path += std::to_string(ThisPlayer->Index);
path += "_";
path += std::to_string((intmax_t)now);
path += ".log";
path /= "/log_of_stratagus_" + std::to_string(ThisPlayer->Index) + "_"
+ std::to_string((intmax_t) now) + ".log";

LogFile = new CFile;
if (LogFile->open(path.c_str(), CL_OPEN_WRITE) == -1) {
if (LogFile->open(path.string().c_str(), CL_OPEN_WRITE) == -1) {
// don't retry for each command
CommandLogDisabled = false;
delete LogFile;
LogFile = nullptr;
return;
}
LastLogFileName = path;
LastLogFileName = path.string();
if (CurrentReplay) {
SaveFullLog(*LogFile);
}
Expand Down Expand Up @@ -909,7 +901,6 @@ int SaveReplay(const std::string &filename)
{
FILE *fd;
char *buf;
std::string destination;
struct stat sb;
size_t size;

Expand All @@ -918,7 +909,7 @@ int SaveReplay(const std::string &filename)
return -1;
}

destination = Parameters::Instance.GetUserDirectory() + "/" + GameName + "/logs/" + filename;
auto destination = Parameters::Instance.GetUserDirectory() / GameName / "logs" / filename;

if (!LastLogFileName.empty() && stat(LastLogFileName.c_str(), &sb)) {
fprintf(stderr, "stat failed\n");
Expand All @@ -938,9 +929,9 @@ int SaveReplay(const std::string &filename)
size = fread(buf, sb.st_size, 1, fd);
fclose(fd);

fd = fopen(destination.c_str(), "wb");
fd = fopen(destination.string().c_str(), "wb");
if (!fd) {
fprintf(stderr, "Can't save to '%s'\n", destination.c_str());
fprintf(stderr, "Can't save to '%s'\n", destination.string().c_str());
delete[] buf;
return -1;
}
Expand Down
Loading

5 comments on commit 8cd05ad

@Andrettin
Copy link
Contributor

Choose a reason for hiding this comment

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

Converting between std::string and std::filesystem::path can unfortunately be problematic for paths with non-ASCII characters, under Windows (if you ever use them in a function like std::filesystem::exists(). This is because under Windows the native encoding of std::filesystem::path is UTF-16, rather than UTF-8.

The fix for that is to use conversion functions which can properly convert between std::string (which in Stratagus we expect to have UTF-8 content) and std::filesystem::path. This can be achieved like this, for example:

namespace path {

inline std::string to_string(const std::filesystem::path &path)
{
#ifdef __clang__
	return path.string();
#else
	//convert a path to a UTF-8 encoded string
	const std::u8string u8str = path.u8string();
	return std::string(u8str.begin(), u8str.end());
#endif
}

inline std::filesystem::path from_string(const std::string &path_str)
{
	try {
#ifdef __clang__
		return std::filesystem::path(path_str);
#else
		//convert a UTF-8 encoded string to a path
		const std::u8string u8str(path_str.begin(), path_str.end());
		return std::filesystem::path(u8str);
#endif
	} catch (...) {
		std::throw_with_nested(std::runtime_error("Failed to convert string \"" + path_str + "\" to a path."));
	}
}

That code is based on utility functions I wrote here:
https://github.com/Andrettin/Archimedes/blob/main/src/util/path_util.h

@Jarod42
Copy link
Contributor Author

@Jarod42 Jarod42 commented on 8cd05ad Aug 4, 2023

Choose a reason for hiding this comment

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

There is (future deprecated) https://en.cppreference.com/w/cpp/filesystem/path/u8path to unify your code.

I have to recheck conversion (but idea is also to decrease the number of such conversions).

thanks.

@Andrettin
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello Joris, thank you!

I was mentioning this just because I found out the hard way that these problems can occur. But it is a really good idea to use std::filesystem::path for paths, it does simplify the code a good deal (I'm also doing it for Wyrmgus).

@Jarod42
Copy link
Contributor Author

@Jarod42 Jarod42 commented on 8cd05ad Aug 5, 2023

Choose a reason for hiding this comment

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

I have check code, and we are not utf-8 everywhere :-/

I think that filesystem::string() (and even u8 version) should be constraint to final usage (fopen-like or display API).

I retest filesystem behavior:

We always have:

std::string s = foo();
assert(filesystem(s).string() == s`

So, I should not have introduce bug with that.
And using u8string() for printf output would improve exactitude :).

@Jarod42
Copy link
Contributor Author

@Jarod42 Jarod42 commented on 8cd05ad Aug 6, 2023

Choose a reason for hiding this comment

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

@Andrettin :
Even more "fun":

  • SDL_RWops uses UTF-8
  • Whereas FILE uses system encoding
  • std::fstream uses system encoding (but accept fs::path).
    DIdin't check bz2/gzip we used...

Please sign in to comment.