Use cross-platform glob library #1562
Conversation
src/common/file_utils.cpp
Outdated
| @@ -2,6 +2,7 @@ | |||
|
|
|||
| #include "common/exception.h" | |||
| #include "common/string_utils.h" | |||
| #include <glob/glob.hpp> | |||
There was a problem hiding this comment.
Since glob.hpp is a third-part library, use "glob/glob.hpp" instead of <>
src/common/file_utils.cpp
Outdated
| glob(path.c_str(), GLOB_TILDE, nullptr, &globResult); | ||
| for (auto i = 0u; i < globResult.gl_pathc; ++i) { | ||
| result.emplace_back(globResult.gl_pathv[i]); | ||
| for (auto &resultPath: glob::glob(path)) { |
There was a problem hiding this comment.
I think it is better to save the result of glob::glob(path) in a local variable, otherwise, we will call glob::glob(path) in each iteration.
There was a problem hiding this comment.
I'm not convinced there's a difference. it should just be using an iterator, initialized at the start by the function call.
Godbolt shows the assembly as being almost identical for gcc (the one difference is before the loop starts), and cppinsights (clang) doesn't show any difference in control flow either.
benjaminwinger
force-pushed
the
cross-platform-glob
branch
from
7e0e821
to
d473e12
Compare
acquamarin
force-pushed
the
cross-platform-glob
branch
from
d473e12
to
722f24e
Compare
acquamarin
force-pushed
the
cross-platform-glob
branch
from
722f24e
to
83092a5
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1562 +/- ##
==========================================
- Coverage 92.00% 91.96% -0.04%
==========================================
Files 700 701 +1
Lines 25116 25204 +88
==========================================
+ Hits 23109 23180 +71
- Misses 2007 2024 +17
☔ View full report in Codecov by Sentry. |
I'll open a larger pull request later with the bulk of the changes required for windows support, but I thought I'd start with a smaller part which can be reviewed independently.
This changes the glob functionality to use https://github.com/p-ranav/glob. It's maintained, and seems to be the best choice out of the cross-platform globbing libraries available (others include this and this, both inactive).
It's probably not as robust as the linux/posix globbing library currently used, and one alternative could be to use this only on windows and use that otherwise, but using the same thing for both is a little simpler and more consistent.
I've included the single header version, using the master version since the latest release is out of date and missing some bugfixes, but I had to revert a commit to fix tilde expansion, which had regressed since the release.
It looks like it supports more or less the same features, with the exception that it does not appear to support username-specific tilde expansion (e.g.
~user->/home/user). I think that it otherwise matches the posix glob.h (at least, with just theGLOB_TILDEextension. Other extensions like braces aren't supported, but they weren't being used anyway).