Better support for / separators on Windows, improve wchar filename coverage #1787
Conversation
|
Crud a test broke - it's currently 4:30am here so I will look at it tomorrow. |
|
All green now! I also fixed an issue where win_eventlog_sink would silently discard errors from Also fixed a security bug in |
| @@ -118,7 +118,7 @@ SPDLOG_INLINE std::tuple<filename_t, filename_t> file_helper::split_by_extension | |||
| } | |||
|
|
|||
| // treat cases like "/etc/rc.d/somelogfile or "/abc/.hiddenfile" | |||
| auto folder_index = fname.rfind(details::os::folder_sep); | |||
| auto folder_index = fname.find_last_of(details::os::folder_seps_filename); | |||
There was a problem hiding this comment.
Will break if folder_seps_filename is more than single char. Let's use the original rfind()
There was a problem hiding this comment.
That's the intention: each character in folder_seps_filename is considered as a possible folder separator. find_last_of searches for any character in folder_seps_filename instead of searching for the entirety of the string.
| { | ||
| result_size = ::MultiByteToWideChar(CP_UTF8, MB_ERR_INVALID_CHARS, str.data(), str_size, NULL, 0); | ||
| } | ||
|
|
||
| if (result_size > 0) | ||
| { | ||
| target.resize(result_size); | ||
| result_size = ::MultiByteToWideChar(CP_UTF8, MB_ERR_INVALID_CHARS, str.data(), str_size, (LPWSTR)target.data(), result_size); | ||
| result_size = ::MultiByteToWideChar(CP_UTF8, MB_ERR_INVALID_CHARS, str.data(), str_size, target.data(), result_size); | ||
|
|
There was a problem hiding this comment.
Why remove the (LPWSTR) cast ? Does target.data() guaranteed to be (LPWSTR) ?
There was a problem hiding this comment.
Yes, because now it uses a wmemory_buffer, which as the w prefix says, is always wide.
The previous cast was questionable (casting from non-wide to wide is a frequent source of bug)
| size_t search_offset = 0; | ||
| do | ||
| { | ||
| auto token_pos = path.find(folder_sep, search_offset); | ||
| auto token_pos = path.find_first_of(folder_seps_filename, search_offset); |
There was a problem hiding this comment.
Will break if folder_seps_filename is more than single char. Let's use the original find()
| #else | ||
| SPDLOG_CONSTEXPR static const char folder_sep = '/'; | ||
| #define SPDLOG_FOLDER_SEPS "/" |
There was a problem hiding this comment.
What would happen if a folder name in windows contains the "\" character ? Let's keep the orig code
There was a problem hiding this comment.
Folder names can't contain this character.
| return rv != nullptr ? rv + 1 : filename; | ||
| // if the size is 2 (1 character + null terminator) we can use the more efficient strrchr | ||
| // the branch will be elided by optimizations | ||
| if (sizeof(os::folder_seps) == 2) |
There was a problem hiding this comment.
Please post a dedicated pr for this optimization.
There was a problem hiding this comment.
This change doesn't make sense without the rest of the folder separator changes
Windows already disallows this and |
|
Thanks @sylveon . Merged. |
The first part of this PR fixes a bug with the default formatting. When not using /FC the
__FILE__macro can take strange values such asC:\Projects\TranslucentTB\TranslucentTB\tray\../dynamicloader.hpp. Under the current code, spdlog would print it in the log as../dynamicloader.hpp. With this PR, it now correctly printsdynamicloader.hpp.As a bonus, I added an extension point for users: they can change what spdlog considers to be a folder separator, if for example they want to revert to the old behavior.
I've also added an AppVeyor worker which tries building spdlog with wide filenames enabled, and adjusted the unit tests accordingly. If I didn't forget anything, I believe it should pass.