Filesystem tests: un-confuse write time

[jfb]

Summary

The filesystem test was confused about access versus write / modification time. The spec says:

file_time_type last_write_time(const path& p, error_code& ec) noexcept;
Returns: The time of last data modification of p, determined as if by the value of the POSIX stat structure member st_mtime obtained as if by POSIX stat(). The signature with argument ec returns file_time_type::min() if an error occurs.

The test was looking at st_atime, not st_mtime, when comparing the result from last_write_time. That was probably due to using a pair instead of naming things nicely or using types. I opted to rename things so it's clearer.

This caused test bot failures.

rdar://problem/40648859

[vsapsai]

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

257–258

We need a comment explaining the intention of this check. Do you know what are the values when the test fails?

I have another (not tested) interpretation of the test that consistently compares write times with write times and access times with access times.

TEST_CHECK(LastAccessTime(file) == file_access_time ||
           LastWriteTime(file) == Clock::to_time_t(ftime2));

I am suspicious about the suggested condition LastWriteTime(file) == file_write_time as I expect it to be false all the time.

[vsapsai]

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

257–258

After checking the last commit that touched this code https://reviews.llvm.org/rL278357 I'm not sure that was a mistake. Looks like the intention was indeed to check LastAccessTime.

What do you think about removing these LastAccessTime checks, including the one for dir? There is no function last_access_time, so we aren't testing the library but idiosyncrasies of file systems.

[EricWF]

So the reason there are atime tests at all is due to the implementation details of __last_write_time here. Essentially we have to do extra work trying to maintain the access time. https://github.com/llvm-mirror/libcxx/blob/master/src/experimental/filesystem/operations.cpp#L855

I'm OK with this patches rename. first and second were certainly not clear.

That being said, I'm not sure we should be removing the access time tests, they were intentional.

[jfb]

• Remove access time checks, simplify existing check, after talking to EricWF on IRC.

[EricWF]

LGTM after addressing inline comments.

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

260

What happened to the test for the directory? Can we add

TEST_CHECK(LastWriteTime(dir) == Clock::to_time_t(dtime2));

[jfb]

• Add back directory test

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

 #include "rapid-cxx-test.hpp"
 #include "filesystem_test_helper.hpp"
 
 #include <sys/stat.h>
 #include <iostream>
 
 using namespace fs;
 
-std::pair<std::time_t, std::time_t> GetTimes(path const& p) {
+struct Times { std::time_t access, write; };
+
+Times GetTimes(path const& p) {
     using Clock = file_time_type::clock;
     struct ::stat st;
     if (::stat(p.c_str(), &st) == -1) {
         std::error_code ec(errno, std::generic_category());
 #ifndef TEST_HAS_NO_EXCEPTIONS
         throw ec;
 #else
         std::cerr << ec.message() << std::endl;
         std::exit(EXIT_FAILURE);
 #endif
     }
     return {st.st_atime, st.st_mtime};
 }
 
 std::time_t LastAccessTime(path const& p) {
-    return GetTimes(p).first;
+    return GetTimes(p).access;
 }
 
 std::time_t LastWriteTime(path const& p) {
-    return GetTimes(p).second;
+    return GetTimes(p).write;
 }
 
 std::pair<std::time_t, std::time_t> GetSymlinkTimes(path const& p) {
     using Clock = file_time_type::clock;
     struct ::stat st;
     if (::lstat(p.c_str(), &st) == -1) {
         std::error_code ec(errno, std::generic_category());
 #ifndef TEST_HAS_NO_EXCEPTIONS
TEST_CASE(get_last_write_time_dynamic_env_test)
     using Clock = file_time_type::clock;
     using Sec = std::chrono::seconds;
     scoped_test_env env;
 
     const path file = env.create_file("file", 42);
     const path dir = env.create_dir("dir");
 
     const auto file_times = GetTimes(file);
-    const std::time_t file_access_time = file_times.first;
-    const std::time_t file_write_time = file_times.second;
+    const std::time_t file_write_time = file_times.write;
     const auto dir_times = GetTimes(dir);
-    const std::time_t dir_access_time = dir_times.first;
-    const std::time_t dir_write_time = dir_times.second;
+    const std::time_t dir_access_time = dir_times.access;
+    const std::time_t dir_write_time = dir_times.write;
 
     file_time_type ftime = last_write_time(file);
     TEST_CHECK(Clock::to_time_t(ftime) == file_write_time);
 
     file_time_type dtime = last_write_time(dir);
     TEST_CHECK(Clock::to_time_t(dtime) == dir_write_time);
 
     SleepFor(Sec(2));
 
     // update file and add a file to the directory. Make sure the times increase.
     std::ofstream of(file, std::ofstream::app);
     of << "hello";
     of.close();
     env.create_file("dir/file1", 1);
 
     file_time_type ftime2 = last_write_time(file);
     file_time_type dtime2 = last_write_time(dir);
 
     TEST_CHECK(ftime2 > ftime);
     TEST_CHECK(dtime2 > dtime);
-    TEST_CHECK(LastAccessTime(file) == file_access_time ||
-               LastAccessTime(file) == Clock::to_time_t(ftime2));
+    TEST_CHECK(LastWriteTime(file) == file_write_time ||
+               LastWriteTime(file) == Clock::to_time_t(ftime2));
     TEST_CHECK(LastAccessTime(dir) == dir_access_time);
 }
 
 
 TEST_CASE(set_last_write_time_dynamic_env_test)
 {
     using Clock = file_time_type::clock;
     using Sec = std::chrono::seconds;
     using Hours = std::chrono::hours;
     using Minutes = std::chrono::minutes;
struct TestCase {
         {dir, past_time},
         {file, before_epoch_time},
         {dir, before_epoch_time},
         {file, just_before_epoch_time},
         {dir, just_before_epoch_time}
     };
     for (const auto& TC : cases) {
         const auto old_times = GetTimes(TC.p);
-        file_time_type old_time(Sec(old_times.second));
+        file_time_type old_time(Sec(old_times.write));
 
         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_CHECK(LastAccessTime(TC.p) == old_times.access);
         }
     }
 }
 
 TEST_CASE(last_write_time_symlink_test)
 {
     using Clock = file_time_type::clock;
     using Sec = std::chrono::seconds;
TEST_CASE(last_write_time_symlink_test)
     std::error_code ec = GetTestEC();
     last_write_time(sym, new_time, ec);
     TEST_CHECK(!ec);
 
     const std::time_t new_time_t = Clock::to_time_t(new_time);
     file_time_type  got_time = last_write_time(sym);
     std::time_t got_time_t = Clock::to_time_t(got_time);
 
-    TEST_CHECK(got_time_t != old_times.second);
+    TEST_CHECK(got_time_t != old_times.write);
     TEST_CHECK(got_time_t == new_time_t);
     TEST_CHECK(LastWriteTime(file) == new_time_t);
-    TEST_CHECK(LastAccessTime(sym) == old_times.first);
+    TEST_CHECK(LastAccessTime(sym) == old_times.access);
     TEST_CHECK(GetSymlinkTimes(sym) == old_sym_times);
 }
 
 
 TEST_CASE(test_write_min_time)
 {
     using Clock = file_time_type::clock;
     using Sec = std::chrono::seconds;