lmi
[Top][All Lists]
Advanced

[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

Attachment: pgpX1xy6wCyF_.pgp
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]