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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai 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());
----------------
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.


https://reviews.llvm.org/D42755