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