[PATCH] D42755: [libcxx] Fix last_write_time tests for filesystems that don't support very small times.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.


================
Comment at: libcxx/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp:360
-
-    ec = GetTestEC();
-    last_write_time(p, Clock::now());
----------------
vsapsai wrote:
> EricWF wrote:
> > I would really love to keep this test. I think it has the ability to detect incorrect integer arithmetic in `last_write_time` when UBSAN is enabled.
> > 
> > Could we maybe add another check like `SupportsAlmostMinTime` where that checks `::min() + MicroSec(1)`?
> What about nesting removed part in `TimeIsRepresentableByFilesystem` like
> ```lang=c++
>     if (TimeIsRepresentableByFilesystem(new_time)) {
>         TEST_CHECK(!ec);
>         TEST_CHECK(tt >= new_time);
>         TEST_CHECK(tt < new_time + Sec(1));
> 
>         ec = GetTestEC();
>         last_write_time(p, Clock::now());
> 
>         new_time = file_time_type::min() + MicroSec(1);
> 
>         last_write_time(p, new_time, ec);
>         tt = last_write_time(p);
> 
>         if (TimeIsRepresentableByFilesystem(new_time)) {
>             TEST_CHECK(!ec);
>             TEST_CHECK(tt >= new_time);
>             TEST_CHECK(tt < new_time + Sec(1));
>         }
>     }
> ```
> 
> As for me, I don't like how it looks. So maybe the same approach but with a flag like
> ```lang=c++
>     bool supports_min_time = false;
>     if (TimeIsRepresentableByFilesystem(new_time)) {
>         TEST_CHECK(!ec);
>         TEST_CHECK(tt >= new_time);
>         TEST_CHECK(tt < new_time + Sec(1));
>         supports_min_time = true;
>     }
> 
>     ec = GetTestEC();
>     last_write_time(p, Clock::now());
> 
>     new_time = file_time_type::min() + MicroSec(1);
> 
>     last_write_time(p, new_time, ec);
>     tt = last_write_time(p);
> 
>     if (supports_min_time && TimeIsRepresentableByFilesystem(new_time)) {
>         TEST_CHECK(!ec);
>         TEST_CHECK(tt >= new_time);
>         TEST_CHECK(tt < new_time + Sec(1));
>     }
> ```
> 
> Yet another option is to leverage that APFS truncates time to supported value, something like
> ```if ((old_tt < new_time + Sec(1)) && TimeIsRepresentableByFilesystem(new_time))```
> I don't like it because it implicitly relies on time truncation which is not guaranteed for all filesystems and it is slightly harder to understand than boolean.
> 
> How do you like these ideas? `SupportsAlmostMinTime` can also work but I'd like to avoid repeating `+ MicroSec(1)` part in 2 places.
The first option seems fine to me.


https://reviews.llvm.org/D42755