Page MenuHomePhabricator

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

Authored by Hahnfeld on Jul 17 2016, 10:37 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

EricWF updated this revision to Diff 64272.Jul 17 2016, 10:37 PM
EricWF retitled this revision from to [libcxx] Fix last_write_time tests for filesystems that don't support negative and very large times..
EricWF updated this object.
EricWF added a subscriber: cfe-commits.
Hahnfeld added inline comments.
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 added inline comments.Jul 18 2016, 12:18 AM
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...

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)?

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...

Thanks. I'll update this patch today.

any progress?

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 commandeered this revision.Dec 20 2016, 5:15 AM
Hahnfeld added a reviewer: EricWF.
Hahnfeld updated this revision to Diff 82102.Dec 20 2016, 5:16 AM
Hahnfeld edited edge metadata.
Hahnfeld removed a subscriber: Hahnfeld.

Version that works for me

Hahnfeld updated this revision to Diff 82803.Jan 2 2017, 6:54 AM

Resolve errors because of recently enabled warnings

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

EricWF accepted this revision.Jan 13 2017, 6:48 PM
EricWF edited edge metadata.

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.

This revision is now accepted and ready to land.Jan 13 2017, 6:48 PM
This revision was automatically updated to reflect the committed changes.

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.