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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
