[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[lmi] PATCH: Switch to using std::filesystem
From: |
Vadim Zeitlin |
Subject: |
[lmi] PATCH: Switch to using std::filesystem |
Date: |
Mon, 26 Apr 2021 23:53:40 +0200 |
Hello,
The branch "std-fs" of https://github.com/vadz/lmi.git repository, also
visible at https://github.com/let-me-illustrate/lmi/pull/180, contains the
long, long promised changes replacing the use of Boost.Filesystem with
std::filesystem.
The main idea here is to keep using fs::path in lmi code, but make it a
thin wrapper around std::filesystem::path rather than an alias for
boost::filesystem::path for the reasons previously discussed in this
thread:
https://lists.nongnu.org/archive/html/lmi/2020-09/msg00049.html
The wrapper class itself is pretty simple, except for char8_t
complications of C++20. I'd rather not go into the details here to avoid
making this post too long, but basically this class always uses
UTF-8-encoded std::string rather than doing different things depending on
the C++ standard version used, as std::filesystem::path does, which seems
like a much saner thing to do, especially because I wouldn't be surprised
at all if the standard behaviour changed once again in C++23.
The rest of the changes just deal with the differences between the two
libraries. They're done in different commits to make it simpler to review
them, but should, for once, be applied all together, i.e. the entire PR
should be "squash merged", as almost all of these commits are needed for
the code to actually compile and work, and preserving their history is not
very important.
Purely for your information, here is a brief description of these changes,
in more or less the commit order:
- initialize_filesystem() is not needed any longer because there is no
requirement to call any functions before using std::fs library, so it was
simply removed. Similarly, path::default_name_check() doesn't exist any
longer and was removed.
- Some Boost functions just don't exist any longer and while we could
provide our own compatibility wrappers, it seems better to switch to the
standard function names to avoid confusion, so here is the list of the
replacements:
- complete() and system_complete() -> absolute()
- basename() -> path::stem() member function
- extension() -> path member function with the same name
- change_extension() -> path::replace_extension() member function
- path::[has_]leaf() -> path::[has_]filename()
- path::branch_path() -> path::parent_path()
- path::native_file_string() -> path::string()
- is_directory() -> directory_entry member function
- directory_entry::path() accessor must be used to get the path
Some of these name changes were also accompanied by the change of the
return type, as all of the path components are now returned as fs::path
and not just strings, so extra calls to string() had to be added in some
places.
- Semantics of some functions have changed too, notably some previously
expected exceptions are not thrown any longer. The tests and the comments
affected by this have been adjusted to match the reality.
More details can be found, as always, in the commit messages, and, of
course, please let me know if you have any questions. And, again, the total
patch is huge, but individual commits should be quite straightforward to
review, so I'd recommend reviewing it commit by commit using the overview
above as a rough guide.
I've tried to test this patch as much as I could and, in addition to the
CI builds (https://github.com/let-me-illustrate/lmi/actions/runs/781455373)
I've also tested it with newer gcc and clang versions locally and we've
also tested it with gcc 10 under native MSW.
BTW, please note that that this patch and the other recently submitted
ones, fixing the build with clang 12[†] and gcc 11[‡], respectively, can be
applied in any order, i.e. this patch doesn't depend on them, and neither
do they depend on it.
[†]: https://lists.nongnu.org/archive/html/lmi/2021-04/msg00046.html
[‡]: https://lists.nongnu.org/archive/html/lmi/2021-04/msg00047.html
Finally, while this patch works, there are other improvements that could
be made in about the same area, and I plan submitting more patches for them
later, this is just an overview of the planned changes:
- Fix strings not using UTF-8 encoding: this isn't really specific to the
paths, but testing has shown that lmi doesn't handle non-ASCII strings as
paths correctly. This had been already the case without this patch, but
it's still the case with it. The problem is not in fs::path itself, but
rather is due to the fact that we don't convert between wxString and
std::string correctly everywhere, i.e. some strings are not actually
UTF-8-encoded as they should be. This is not difficult to fix, but,
again, I'd rather do it in a separate patch.
- Pretty minor, but now that we require C++20, we really ought to define
operator<=>() rather than 6 relational operators for fs::path, as is
currently done because we only required C++17 when this code was written.
But then it probably makes sense to do this for the other classes with
custom comparison operators too, so, again, I'd rather do it separately.
- This is more speculative, but we could implement our own validation of
paths for MSW, now that fs::path doesn't do it any longer. I'm not quite
sure if we really need to do it, so I'd prefer to discuss (and do, if
necessary), this later too.
Sorry again for the inordinate delay with submitting this patch, which was
mostly done several months ago, and I hope that at least the final version
is now good enough to be applied.
Thanks in advance for looking at it!
VZ
pgpX1xy6wCyF_.pgp
Description: PGP signature