Repository navigation
Conversation
Signed-off-by: Adrien Gallouët <angt@huggingface.co>
8011b02 to
34477a5
Compare
| void common_log_set_prefix (struct common_log * log, bool prefix); // whether to output prefix to each log | ||
| void common_log_set_timestamps(struct common_log * log, bool timestamps); // whether to output timestamps in the prefix | ||
| void common_log_flush (struct common_log * log); // flush all pending log messages | ||
| void common_log_set_file (struct common_log * log, const std::filesystem::path & file); // not thread-safe |
There was a problem hiding this comment.
The other changes are OK, but here I don't see a reason to change to std::filesystem::path - it results in <filesystem> getting included in a lot of files for no benefit (in the end we still end up using path.c_str() in set_file).
There was a problem hiding this comment.
The point of fs::path is that path.c_str() returns the native path (const wchar_t* on Windows), so set_file can pass it directly to _wfopen without any conversion at all.
Also, when the path comes from the env (not from args for now), we keep the native utf16 string as is. With std::string we would have to go utf16 -> utf8 -> utf16, which can lose information on Windows (not common at all, but yes).
So after many iterations I ended up using fs::path to "hide" the special Windows case and avoid these errors in the future.
There was a problem hiding this comment.
The point of fs::path is that path.c_str() returns the native path (const wchar_t* on Windows), so set_file can pass it directly to _wfopen without any conversion at all.
Is there a problem to make the conversion in set_file? We would contain the <filesystem> header to the log.cpp file which is OK.
There was a problem hiding this comment.
Yes we can, so we go for u8path() everywhere ?
There was a problem hiding this comment.
In that case, I can make a new PR, just converting all env to utf8, forgetting about the fs::path direction.
There was a problem hiding this comment.
Yes, I think it will be better.
There was a problem hiding this comment.
or maybe add our own wrapper common_fs ?
I feel like fs-related ops like read/write files, as well as utf8 path handling is currently not very well organized in the code base
There was a problem hiding this comment.
I think a big part of the issue comes down to using std::string for paths on Windows, they're two different beasts and we end up using fs::path repeatedly and internally just to convert them.
Also, moving entirely to utf8 has proven to be error-prone in my experience. People (myself included) will forget to use u8path() when interacting with the fs.
In modern C++, using fs::path is the way to go to avoid that, no need to convert anything.
Overview
u8path()).--log-fileon Windows (as a first iteration).Additional information
Part of the ongoing
fs::pathmodernization seriesRequirements