Re: [lmi] PATCH: Switch to using std::filesystem

[Greg Chicares]

On 4/26/21 9:53 PM, Vadim Zeitlin wrote:
> 
>  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.

I've reviewed all these changes, line by line, and pushed some
modifications of my own, most notably rewriting some of the
'path_utility*.?pp' code, originally implemented in terms of
boost::filesystem and now of std::filesystem.

>  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.

Agreed.

> - 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.

Agreed.

> - 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.

Yes, that's perhaps the biggest difference between the boost original
and the standard version. When users enter paths using OS-standard
dialog controls, those controls reject names that the OS forbids, so
maybe boost's validation didn't accomplish much. OTOH, the rules are
not very complicated, and Beman Dawes has set them out clearly:
  
https://www.boost.org/doc/libs/1_33_1/libs/filesystem/doc/portability_guide.htm
so maybe we should implement them in lmi, in order to make validity
an explicit precondition.

In addition to the two sets of {posix, msw} OS-specific requirements,
that page also makes portability recommendations, which are at least
partly the same as what lmi's portable_filename() does. I think it's
a good idea to enforce at least some rules, lest an end user choose
file names such as:
  C:\My Stuff\An XYZ Corp Census.cns
  C:\My Stuff\...and another XYZ Corp Census.cns

Here's one concrete example that just occurred to me:
  File | New | Census
  File | Save as...
  enter "Yet another *new* census?" without the double-quotes
  click the Save button
At least with wine-5.x, nothing seems to happen: no file is saved,
and the dialog does not close. But if that happens entirely within
a standard dialog, there's probably nothing lmi can do about it
anyway.

OTOH, testing the triple-dot example above gives
  Warning: Unable to save 'Z:\opt\lmi.and another XYZ Corp Census.cns'.
  [census_document.cpp : 94]
and that's probably something lmi should trap.