Replace fs::relative call with custom GetRelativePath#14531
Replace fs::relative call with custom GetRelativePath#14531promag wants to merge 6 commits intobitcoin:masterfrom
Conversation
src/wallet/walletutil.cpp
Outdated
There was a problem hiding this comment.
please add a unit test for this function
promag
force-pushed
the
2018-10-getrelativepath
branch
4 times, most recently
from
73838a3 to
ca5df04
Compare
promag
force-pushed
the
2018-10-getrelativepath
branch
2 times, most recently
from
8a4d526 to
ad779bc
Compare
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
Note to reviewers, I've removed the set() because it removes duplicate and that's not the point — w7 was duplicate — sorted() is what's needed.
src/util.cpp
Outdated
There was a problem hiding this comment.
On Windows, std::filesystem::path("C:\\").parent_path() == std::filesystem::path("C:\\"). It would be an infinity loop. (Only test std::filesystem in MSVC, doesn't test boost::filesystem)
There was a problem hiding this comment.
Added parent != parent.root_path() condition to the loop.
There was a problem hiding this comment.
On Windows, std::filesystem::path("C:\").parent_path() == std::filesystem::path("C:\")
Workaround seems ok, but is this is a bug? Both boost filesystem and std::filesystem are supposed to return empty if the path contains a single element:
https://en.cppreference.com/w/cpp/experimental/fs/path/parent_path
https://www.boost.org/doc/libs/1_68_0/libs/filesystem/doc/reference.html#path-parent_path
There was a problem hiding this comment.
https://en.cppreference.com/w/cpp/filesystem/path/parent_path
The C++17 standard one is a little different from the experimental one.
src/test/util_tests.cpp
Outdated
There was a problem hiding this comment.
I assume this would cause infinity loop on Windows if base / "bar" exist.
GetRelativePath(path, base, base / "bar");
src/test/util_tests.cpp
Outdated
There was a problem hiding this comment.
I had that but datadir seemed unrelated to this function. I can revert if there is a reason to do that.
The implementation of fs::relative resolves symlinks which is not intended in ListWalletDir. The replacement does what is required, and listwalletdir tests are fixed accordingly. Also, building with boost 1.47 required 2 changes: - replace fs::relative with an alternative implementation; - fix fs::recursive_directory_iterator iteration.
promag
force-pushed
the
2018-10-getrelativepath
branch
from
ad779bc to
8697d65
Compare
|
Alternative approach in #14561. |
| env = nullptr; | ||
| } else { | ||
| // To avoid accessing a map that has already deconstructed, do not call this when shutdown is true. g_dbenvs.erase would also erase this value. | ||
| // TODO: get rid of wild pointers |
There was a problem hiding this comment.
This is based on #14559, should comment there.
|
Would suggest closing this. I think the fix in #14561 is simpler and more correct. |
|
Agree @ryanofsky. |
7 participants
The implementation of
fs::relativeresolves symlinks which is not intendedin ListWalletDir. The replacement does what is required, and
listwalletdirRPCtests are fixed accordingly.
Also,
fs::recursive_directory_iteratoriteration is fixed to build with boost 1.47.