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

[Greg Chicares]

On 5/10/21 2:28 PM, Vadim Zeitlin wrote:
> On Mon, 3 May 2021 12:21:29 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
[...]
> GC> > - Fix strings not using UTF-8 encoding
> [...]
> GC> Agreed.
> GC> 
> GC> > - Pretty minor, but now that we require C++20, we really ought to define
> GC> >   operator<=>() rather than 6 relational operators for fs::path
> [...]
> GC> Agreed.
> 
>  My short-term TODO list is then:
> 
> 0. Fix the CI builds (I feel acutely uncomfortable without the safety they
>    provide, so for me this is the highest priority).

Agreed. All automated tests must always succeed.

> 1. Use operator<=>() for fs::path -- and maybe other classes? Do you think
>    I should restrict my patch to fs::path only or also use it for the other
>    classes, such as:

It seems best to do them all at once.

>  - calendar_date (which, surprisingly, has 4 out of 6 operators right now)

That's because I consciously avoid writing '>' or '>=', preferring
to write all comparisons in number-line order. Thus, I've never
cared about the two missing operators. But if we get six for the
price of one (or maybe it's for the price of zero), that's great.

>  - currency

No objection.

>  - minmax (which defines 2 of 6 comparison operators)

Again, that's good enough for comparisons in number-line order.
I can't think of a good use case for operator== or operator!=
here. But six for the price of one (or zero) is a good deal.

>  - soa_v3_format::table::Number (you probably won't mind me changing this one)

Fine by me.

>  - TypeInfo (which is admittedly a rather special case)

C++20 [type.info] defines only
  operator==()
  before()
which map to
  TypeInfo::operator==()
  TypeInfo::operator<()
I guess I wrote both just because I could. If I obliterate the
second one, lmi still builds, along with all its unit tests
except the one that tests operator<(). I don't know whether
there's any reason other than symmetry to provide the other
relational operators; but if we get all of them trivially,
I guess that's fine. Six trivially is better than one with
effort; yet can the compiler handle this class's comparisons
trivially?

>  - tn_range (which defines 3 of 6 comparison operators)

Same reasoning as for minmax, I suppose.

> 2. UTF-8 support for paths.

Agreed. And, BTW--I can't remember if I asked--what do you think
of allowing UTF-8 in source files, for comments at least? I suppose
that would work today if we just removed the 'test_coding_rules'
code that forbids it.

> GC> > - This is more speculative, but we could implement our own validation of
> GC> >   paths for MSW, now that fs::path doesn't do it any longer. I'm not 
> quite
> GC> >   sure if we really need to do it, so I'd prefer to discuss (and do, if
> GC> >   necessary), this later too.
> GC> 
> GC> Yes, that's perhaps the biggest difference between the boost original
> GC> and the standard version. When users enter paths using OS-standard
> GC> dialog controls, those controls reject names that the OS forbids, so
> GC> maybe boost's validation didn't accomplish much.
> 
>  FWIW I've never seen its effect or maybe missed it if I did.
[...]
>  I wonder what do we gain by doing it. In the worst case, we're just going
> to get an error when trying to actually use the file. Is it really much
> worse than giving a (possibly spurious!) error before trying to use it?

Agreed. Let's forget about doing that.

> GC> In addition to the two sets of {posix, msw} OS-specific requirements,
> GC> that page also makes portability recommendations, which are at least
> GC> partly the same as what lmi's portable_filename() does. I think it's
> GC> a good idea to enforce at least some rules, lest an end user choose
> GC> file names such as:
> GC>   C:\My Stuff\An XYZ Corp Census.cns
> GC>   C:\My Stuff\...and another XYZ Corp Census.cns
> 
>  Sorry, why is this a problem? Periods are not forbidden in the file names
> and if the user really wants to use a file called like this, why shouldn't
> they be able to do it?

My real concern is the second, with an initial series of dots: see
discussion below.

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

I guess our answer to any end-user complaint is simply:
  "Try saving a file by that name in 'excel'."

As for a series of initial dots:

> GC> OTOH, testing the triple-dot example above gives
> GC>   Warning: Unable to save 'Z:\opt\lmi.and another XYZ Corp Census.cns'.
> GC>   [census_document.cpp : 94]
> GC> and that's probably something lmi should trap.
> 
>  No, I think it's just a bug. But the problem is that I couldn't reproduce
> it here with my existing MSVC build, i.e. the file gets saved without any
> errors. I'll retest with MinGW binary slightly later, but I think this
> might be actually something Wine-specific rather than a compiler-dependent
> problem. And if this is really true, the impetus for doing anything here
> becomes even smaller and, IMO, non-existent.

Agreed.

>  To summarize, right now I don't see any reason to implement file path
> validation

Agreed.