[core] Refactor ecal_path_processing.cpp to C++17 std::filesystem#2608
[core] Refactor ecal_path_processing.cpp to C++17 std::filesystem#2608
ecal_path_processing.cpp to C++17 std::filesystem#2608Conversation
hannemn
left a comment
There was a problem hiding this comment.
@FlorianReimold Can you give the PR a short review please? Thanks. :-)
FlorianReimold
left a comment
There was a problem hiding this comment.
This is my first review. I will perform another review regarding Unicode correctness. Please stay tuned 😉
| testFile.close(); | ||
| std::remove(testFilePath.c_str()); | ||
| test_file.close(); | ||
| std::filesystem::remove(test_file_path); |
There was a problem hiding this comment.
I feel like exceptions should be caught or the error_code should be passed, so that a failure while removing the dummy test file does not crash the application.
|
|
||
| char unique_path[MAX_PATH]; | ||
| if (!GetTempFileNameA(tmp_dir.c_str(), "ecal", 0, unique_path) != 0) | ||
| if (GetTempFileNameA(tmp_dir.c_str(), "ecal", 0, unique_path) == 0) |
There was a problem hiding this comment.
Using hardcoded ANSI functions it probably is fine in 99.9% of all cases, as the tmp dir will most likely only consist of 7-bit ASCII characters anyways. However, it it doesn't, this will fail, as the ANSI output is later interpreted as UTF-8 (I think).
Thus, the proper way would be to use the hardcoded Unicode equivalent and convert that to UTF-8.
I found hardcoded ANSI functions in the following functions:
getTempDir
GetTempPathA
DirProvider::uniqueTmpDir
GetTempFileNameADeleteFileACreateDirectoryA
|
Ok, so I did a small test and come to the conclusion that the current implementation does not handle Unicode paths in Windows. Take the following code for example: ecal/ecal/core/src/config/ecal_path_processing.cpp Lines 356 to 360 in 92f9973 When any eCAL App is being started from a path with unicode characters that don't appear in the currently configure codepage (in my test Therefore, a deeper analysis has to be done to replace all non-widestring calls with widestring equivalents on windows. |
|
I conducted a manual review of the PR changes and didn't find any issues. I think, it should now handle Unicode data properly. I will however continue with a small review of the non-changed codebase as well so see if the surrounding code handles it properly. |
FlorianReimold
left a comment
There was a problem hiding this comment.
OK, looks good for me. Widestring and UTF 8 was used consistently everywhere I checked. An AI review told me the same. The AI review mentioned that we are currently not supporting long file paths for the temp directory (it uses MAX_PATH) and that there are ways to also support long paths there. But I think the current state is fine.
Description
Replaces the custom
EcalUtils::Filesystem/EcalUtils::Stringwrappers inecal_path_processing.cppwith standard C++17<filesystem>equivalents and fixes several bugs found during the review.