fileutil: use std::filesystem #11426
Conversation
|
Poke on this? |
| int prev_depth = 0; | ||
| std::stack<FSTEntry*> dir_fsts; | ||
| dir_fsts.push(&parent_entry); | ||
| for (auto it = fs::recursive_directory_iterator(directory_path, error); |
There was a problem hiding this comment.
This segment comes across as a bit confusing with the stack and the depth comparisons. Am I right in assuming we are just trying to come up with a tree of FSTEntrys? And that we can't rely on recursive_directory_iterator alone because its order is undefined?
If so, we could improve things from a readability standpoint (not sure if it's more expensive) by avoiding it altogether:
void RecurseTreeHelper(auto& fst_entry, auto fs_path)
{
for (auto it = fs::directory_iterator(fs_path, error); it != fs::directory_iterator();
it.increment(error))
{
auto child_entry = dirent_to_fstent(*it);
RecurseTreeHelper(child_entry, *it);
fst_entry.children.emplace_back(std::move(child_entry));
}
fst_entry.size += fst_entry.children.size();
for (auto& child : fst_entry.children)
if (child.isDirectory)
fst_entry.size += child.size;
}
Could also do this with a lambda but it's a little klunky: https://stackoverflow.com/questions/2067988/recursive-lambda-functions-in-c11
If I'm completely missing the point, let me know.
There was a problem hiding this comment.
The main complication I was working around is that dolphin expects to return the size of all entries (see calc_dir_size), so you need to account for that as well.
Also:
- you must be careful that you don't cause some iterators to become invalidated that will be used in the future.
- I also generally try to avoid recursion where possible, and here the stack seems pretty normal way to do that. I don't feel super strongly about it here, but I'm probably not going to change it - maybe change it in a future PR if you'd like?
There was a problem hiding this comment.
Yeah, I didn't take into account the dir size. Should be easy enough to fix (EDIT: possibly fixed now) but I get your points. Out of curiosity, do you avoid recursion due to concerns about stack size or just not liking it? (I've seen both)
FWIW, I do see what you're doing and it seems like it should work but it isn't intuitive enough to me that I can readily sign off on it. Unfortunately, we only have unit tests for deletion otherwise I'd have a bit more confidence. But yeah this is pretty minor, the code overall looks really good. Great work!
There was a problem hiding this comment.
Yes, mainly stack size. It's much easier to accidentally run out of stack than heap.
when i made the commit, i made a unittest that compared an fstentry tree of my entire C:\ drive using the old+new code and it was identical, altho it's just a random test :)
There was a problem hiding this comment.
Looks fine to me. I do agree with iwubcode that it would be good to have unit tests for this stuff so we can know it doesn't change behavior, but what can you do...
I'm curious why you removed that GetExePath() optimization but otherwise I think we should just merge this and hope for the best.
| static const std::string dolphin_path = [] { | ||
| std::string result; | ||
| #ifdef _WIN32 | ||
| auto dolphin_exe_path = GetModuleName(nullptr); | ||
| if (dolphin_exe_path) | ||
| { | ||
| std::unique_ptr<TCHAR[], decltype(&std::free)> dolphin_exe_expanded_path{ | ||
| _tfullpath(nullptr, dolphin_exe_path->c_str(), 0), std::free}; | ||
| if (dolphin_exe_expanded_path) | ||
| { | ||
| result = TStrToUTF8(dolphin_exe_expanded_path.get()); | ||
| } | ||
| else | ||
| { | ||
| result = TStrToUTF8(*dolphin_exe_path); | ||
| } | ||
| } | ||
| auto exe_path = GetModuleName(nullptr); | ||
| if (!exe_path) | ||
| return {}; | ||
| std::error_code error; | ||
| auto exe_path_absolute = fs::absolute(exe_path.value(), error); | ||
| if (error) | ||
| return {}; | ||
| return PathToString(exe_path_absolute); | ||
| #elif defined(__APPLE__) | ||
| result = GetBundleDirectory(); | ||
| return GetBundleDirectory(); | ||
| #else | ||
| char dolphin_exe_path[PATH_MAX]; | ||
| ssize_t len = ::readlink("/proc/self/exe", dolphin_exe_path, sizeof(dolphin_exe_path)); | ||
| if (len == -1 || len == sizeof(dolphin_exe_path)) | ||
| { | ||
| len = 0; | ||
| } | ||
| dolphin_exe_path[len] = '\0'; | ||
| result = dolphin_exe_path; | ||
| char dolphin_exe_path[PATH_MAX]; | ||
| ssize_t len = ::readlink("/proc/self/exe", dolphin_exe_path, sizeof(dolphin_exe_path)); | ||
| if (len == -1 || len == sizeof(dolphin_exe_path)) | ||
| { | ||
| len = 0; | ||
| } | ||
| dolphin_exe_path[len] = '\0'; | ||
| return dolphin_exe_path; | ||
| #endif | ||
| return result; | ||
| }(); | ||
| return dolphin_path; |
There was a problem hiding this comment.
Is there a reason you removed this construct? As far as I can tell this is supposed to be an optimization so the exe path is only constructed once instead of on every call to GetExePath(), but granted I don't know how critical this optimization actually is. It's been there since forever.
There was a problem hiding this comment.
Mostly I just thought the lambda-into-static was ugly. On windows I dont think it provides any important speedup as the data is just fetched from the PEB and so at most you pay a string conversion/copy cost (and afaik it's not called often). But yea, caching it isn't terrible in itself.
|
Well I tested this for a few minutes and everything seems to work as expected, so here goes nothing... |
replaces most stuff
incorporates #10955
needs #10994 to be merged first for platforms that didn't previously use std::filesystem to compile