[libcxx] Fix last_write_time tests for filesystems that don't support negative and very large times.

[Hahnfeld]

test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp

113–119 ↗

(On Diff #64272)

Hmm no, this doesn't work for me: fs::last_write_time just takes the negative timestamp and overflows somewhere internally...

[Hahnfeld]

test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp

89–95 ↗

(On Diff #64272)

Obviously, I meant this part of the code, sorry:

Hmm no, this doesn't work for me: fs::last_write_time just takes the negative timestamp and overflows somewhere internally...

[Hahnfeld]

Any chance we can get this fixed?

test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp

89–95 ↗

(On Diff #64272)

Maybe we can use if (ec && new_write_time <= -5)?

[Hahnfeld]

In addition to this patch, my local environment would require the following changes:

diff --git a/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp b/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
index 01b3a67..a01b386 100644
--- a/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
+++ b/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
@@ -86,13 +86,7 @@ bool TestSupportsNegativeTimes() {
         fs::last_write_time(file, tp, ec);
         new_write_time = LastWriteTime(file);
     }
-    if (ec) {
-        assert(old_write_time == new_write_time);
-        return false;
-    } else {
-        assert(new_write_time <= -5);
-        return true;
-    }
+    return !ec && new_write_time <= -5;
 }
 
 bool TestSupportsMaxTime() {
@@ -110,13 +104,7 @@ bool TestSupportsMaxTime() {
         fs::last_write_time(file, tp, ec);
         new_write_time = LastWriteTime(file);
     }
-    if (ec) {
-        assert(new_write_time == old_write_time);
-        return false;
-    } else {
-        assert(new_write_time > old_write_time);
-        return true;
-    }
+    return !ec && new_write_time > max_sec - 1;
 }
 
 static const bool SupportsNegativeTimes = TestSupportsNegativeTimes();
@@ -133,8 +121,9 @@ inline bool TimeIsRepresentableByFilesystem(file_time_type tp) {
     using namespace std::chrono;
     using Lim = std::numeric_limits<std::time_t>;
     auto sec = duration_cast<seconds>(tp.time_since_epoch()).count();
+    auto microsec = duration_cast<microseconds>(tp.time_since_epoch()).count();
     if (sec < Lim::min() || sec > Lim::max())   return false;
-    else if (sec < 0 && !SupportsNegativeTimes) return false;
+    else if (microsec < 0 && !SupportsNegativeTimes) return false;
     else if (tp == file_time_type::max() && !SupportsMaxTime) return false;
     return true;
 }
@@ -273,15 +262,17 @@ TEST_CASE(set_last_write_time_dynamic_env_test)
 
         file_time_type  got_time = last_write_time(TC.p);
 
-        TEST_CHECK(got_time != old_time);
-        if (TC.new_time < epoch_time) {
-            TEST_CHECK(got_time <= TC.new_time);
-            TEST_CHECK(got_time > TC.new_time - Sec(1));
-        } else {
-            TEST_CHECK(got_time <= TC.new_time + Sec(1));
-            TEST_CHECK(got_time >= TC.new_time - Sec(1));
+        if (TimeIsRepresentableByFilesystem(TC.new_time)) {
+            TEST_CHECK(got_time != old_time);
+            if (TC.new_time < epoch_time) {
+                TEST_CHECK(got_time <= TC.new_time);
+                TEST_CHECK(got_time > TC.new_time - Sec(1));
+            } else {
+                TEST_CHECK(got_time <= TC.new_time + Sec(1));
+                TEST_CHECK(got_time >= TC.new_time - Sec(1));
+            }
+            TEST_CHECK(LastAccessTime(TC.p) == old_times.first);
         }
-        TEST_CHECK(LastAccessTime(TC.p) == old_times.first);
     }
 }
 
@@ -334,11 +325,7 @@ TEST_CASE(test_write_min_time)
     last_write_time(p, new_time, ec);
     file_time_type tt = last_write_time(p);
 
-    if (!TimeIsRepresentableByFilesystem(new_time)) {
-        TEST_CHECK(ec);
-        TEST_CHECK(ec != GetTestEC());
-        TEST_CHECK(tt == last_time);
-    } else {
+    if (TimeIsRepresentableByFilesystem(new_time)) {
         TEST_CHECK(!ec);
         TEST_CHECK(tt >= new_time);
         TEST_CHECK(tt < new_time + Sec(1));
@@ -353,11 +340,7 @@ TEST_CASE(test_write_min_time)
     last_write_time(p, new_time, ec);
     tt = last_write_time(p);
 
-    if (!TimeIsRepresentableByFilesystem(new_time)) {
-        TEST_CHECK(ec);
-        TEST_CHECK(ec != GetTestEC());
-        TEST_CHECK(tt == last_time);
-    } else {
+    if (TimeIsRepresentableByFilesystem(new_time)) {
         TEST_CHECK(!ec);
         TEST_CHECK(tt >= new_time);
         TEST_CHECK(tt < new_time + Sec(1));
@@ -383,11 +366,7 @@ TEST_CASE(test_write_min_max_time)
     last_write_time(p, new_time, ec);
     file_time_type tt = last_write_time(p);
 
-    if (!TimeIsRepresentableByFilesystem(new_time)) {
-        TEST_CHECK(ec);
-        TEST_CHECK(ec != GetTestEC());
-        TEST_CHECK(tt == last_time);
-    } else {
+    if (TimeIsRepresentableByFilesystem(new_time)) {
         TEST_CHECK(!ec);
         TEST_CHECK(tt > new_time - Sec(1));
         TEST_CHECK(tt <= new_time);

However, this disables some of the tests if the filesystem behaves wrongly...

[EricWF]

Thanks. I'll update this patch today.

[Hahnfeld]

ping?

[Hahnfeld]

any progress?

[EricWF]

Sorry no progress yet. I've been working on bigger bugs relating to our handling of file times. Once I get those worked out I'll fix this.

[Hahnfeld]

Version that works for me

[Hahnfeld]

Resolve errors because of recently enabled warnings

[Hahnfeld]

Ping. Would this be ok to land before branching for 4.0 this week?

[Hahnfeld]

@EricWF ?

[EricWF]

LGTM. Do you need somebody to commit this for you?

Let me know once you've committed it and I'll merge it into the 4.0 branch.

[Hahnfeld]

In D22452#646165, @EricWF wrote:

Let me know once you've committed it and I'll merge it into the 4.0 branch.

I've replied to the commit mail and asked Hans.

libcxx/trunk/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp

 #else
         std::cerr << ec.message() << std::endl;
         std::exit(EXIT_FAILURE);
 #endif
     }
     return {st.st_atime, st.st_mtime};
 }
 
-inline bool TimeIsRepresentableAsTimeT(file_time_type tp) {
+namespace {
+bool TestSupportsNegativeTimes() {
+    using namespace std::chrono;
+    std::error_code ec;
+    std::time_t old_write_time, new_write_time;
+    { // WARNING: Do not assert in this scope.
+        scoped_test_env env;
+        const path file = env.create_file("file", 42);
+        old_write_time = LastWriteTime(file);
+        file_time_type tp(seconds(-5));
+        fs::last_write_time(file, tp, ec);
+        new_write_time = LastWriteTime(file);
+    }
+    return !ec && new_write_time <= -5;
+}
+
+bool TestSupportsMaxTime() {
     using namespace std::chrono;
     using Lim = std::numeric_limits<std::time_t>;
-    auto sec = duration_cast<seconds>(tp.time_since_epoch()).count();
-    return (sec >= Lim::min() && sec <= Lim::max());
+    auto max_sec = duration_cast<seconds>(file_time_type::max().time_since_epoch()).count();
+    if (max_sec > Lim::max()) return false;
+    std::error_code ec;
+    std::time_t old_write_time, new_write_time;
+    { // WARNING: Do not assert in this scope.
+        scoped_test_env env;
+        const path file = env.create_file("file", 42);
+        old_write_time = LastWriteTime(file);
+        file_time_type tp = file_time_type::max();
+        fs::last_write_time(file, tp, ec);
+        new_write_time = LastWriteTime(file);
+    }
+    return !ec && new_write_time > max_sec - 1;
 }
 
+static const bool SupportsNegativeTimes = TestSupportsNegativeTimes();
+static const bool SupportsMaxTime = TestSupportsMaxTime();
+
+} // end namespace
+
+// Check if a time point is representable on a given filesystem. Check that:
+// (A) 'tp' is representable as a time_t
+// (B) 'tp' is non-negative or the filesystem supports negative times.
+// (C) 'tp' is not 'file_time_type::max()' or the filesystem supports the max
+//     value.
+inline bool TimeIsRepresentableByFilesystem(file_time_type tp) {
+    using namespace std::chrono;
+    using Lim = std::numeric_limits<std::time_t>;
+    auto sec = duration_cast<seconds>(tp.time_since_epoch()).count();
+    auto microsec = duration_cast<microseconds>(tp.time_since_epoch()).count();
+    if (sec < Lim::min() || sec > Lim::max())   return false;
+    else if (microsec < 0 && !SupportsNegativeTimes) return false;
+    else if (tp == file_time_type::max() && !SupportsMaxTime) return false;
+    return true;
+}
 
 TEST_SUITE(exists_test_suite)
 
 TEST_CASE(signature_test)
 {
     const file_time_type t;
     const path p; ((void)p);
     std::error_code ec; ((void)ec);
for (const auto& TC : cases) {
         file_time_type old_time(Sec(old_times.second));
 
         std::error_code ec = GetTestEC();
         last_write_time(TC.p, TC.new_time, ec);
         TEST_CHECK(!ec);
 
         file_time_type  got_time = last_write_time(TC.p);
 
+        if (TimeIsRepresentableByFilesystem(TC.new_time)) {
             TEST_CHECK(got_time != old_time);
             if (TC.new_time < epoch_time) {
                 TEST_CHECK(got_time <= TC.new_time);
                 TEST_CHECK(got_time > TC.new_time - Sec(1));
             } else {
                 TEST_CHECK(got_time <= TC.new_time + Sec(1));
                 TEST_CHECK(got_time >= TC.new_time - Sec(1));
             }
             TEST_CHECK(LastAccessTime(TC.p) == old_times.first);
         }
     }
+}
 
 TEST_CASE(last_write_time_symlink_test)
 {
     using Clock = file_time_type::clock;
     using Sec = std::chrono::seconds;
     using Hours = std::chrono::hours;
     using Minutes = std::chrono::minutes;
 
TEST_CASE(test_write_min_time)
     using Clock = file_time_type::clock;
     using Sec = std::chrono::seconds;
     using MicroSec = std::chrono::microseconds;
     using Lim = std::numeric_limits<std::time_t>;
     scoped_test_env env;
     const path p = env.create_file("file", 42);
 
     std::error_code ec = GetTestEC();
-    file_time_type last_time = last_write_time(p);
     file_time_type new_time = file_time_type::min();
 
     last_write_time(p, new_time, ec);
     file_time_type tt = last_write_time(p);
 
-    if (!TimeIsRepresentableAsTimeT(new_time)) {
-        TEST_CHECK(ec);
-        TEST_CHECK(ec != GetTestEC());
-        TEST_CHECK(tt == last_time);
-    } else {
+    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());
-    last_time = last_write_time(p);
 
     new_time = file_time_type::min() + MicroSec(1);
 
     last_write_time(p, new_time, ec);
     tt = last_write_time(p);
 
-    if (!TimeIsRepresentableAsTimeT(new_time)) {
-        TEST_CHECK(ec);
-        TEST_CHECK(ec != GetTestEC());
-        TEST_CHECK(tt == last_time);
-    } else {
+    if (TimeIsRepresentableByFilesystem(new_time)) {
         TEST_CHECK(!ec);
         TEST_CHECK(tt >= new_time);
         TEST_CHECK(tt < new_time + Sec(1));
     }
 }
 
 
 
 TEST_CASE(test_write_min_max_time)
 {
     using Clock = file_time_type::clock;
     using Sec = std::chrono::seconds;
     using Hours = std::chrono::hours;
     using Lim = std::numeric_limits<std::time_t>;
     scoped_test_env env;
     const path p = env.create_file("file", 42);
 
     std::error_code ec = GetTestEC();
-    file_time_type last_time = last_write_time(p);
     file_time_type new_time = file_time_type::max();
 
     ec = GetTestEC();
     last_write_time(p, new_time, ec);
     file_time_type tt = last_write_time(p);
 
-    if (!TimeIsRepresentableAsTimeT(new_time)) {
-        TEST_CHECK(ec);
-        TEST_CHECK(ec != GetTestEC());
-        TEST_CHECK(tt == last_time);
-    } else {
+    if (TimeIsRepresentableByFilesystem(new_time)) {
         TEST_CHECK(!ec);
         TEST_CHECK(tt > new_time - Sec(1));
         TEST_CHECK(tt <= new_time);
     }
 }
 
 TEST_CASE(test_value_on_failure)
 {