diff mbox

Five patches for std::experimental::filesystem

Message ID 20161027110139.GY2922@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely Oct. 27, 2016, 1:34 p.m. UTC 
On 26/10/16 09:24 +0200, Christophe Lyon wrote:
>Hi Jonathan,

>

>On 25 October 2016 at 17:32, Jonathan Wakely <jwakely@redhat.com> wrote:

>> Two more fixes for the filesystem TS, and improved tests.

>>

>>   Handle negative times in filesystem::last_write_time

>>           * src/filesystem/ops.cc

>>        (last_write_time(const path&, file_time_type, error_code&)): Handle

>>        negative times correctly.

>>        * testsuite/experimental/filesystem/operations/last_write_time.cc:

>>        Test writing file times.

>>

>>    Fix error handling in copy_file and equivalent

>>           * src/filesystem/ops.cc (do_copy_file): Report an error if source

>> or

>>        destination is not a regular file (LWG 2712).

>>        (equivalent): Fix error handling and result when only one file

>> exists.

>>        * testsuite/experimental/filesystem/operations/copy.cc: Remove files

>>        created by tests. Test copying directories.

>>        * testsuite/experimental/filesystem/operations/copy_file.cc: Remove

>>        files created by tests.

>>        * testsuite/experimental/filesystem/operations/equivalent.cc: New.

>>        * testsuite/experimental/filesystem/operations/is_empty.cc: New.

>>        * testsuite/experimental/filesystem/operations/read_symlink.cc:

>> Remove

>>        file created by test.

>>        * testsuite/experimental/filesystem/operations/remove_all.cc: New.

>>        * testsuite/util/testsuite_fs.h (~scoped_file): Only try to remove

>>        file if path is non-empty, to support removal by other means.

>>

>> Tested x86_64-linux, committed to trunk.

>>

>>

>I can see failures in

>experimental/filesystem/operations/last_write_time.cc after your

>committed this patch:

>/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/testsuite/experimental/filesystem/operations/last_write_time.cc:127:

>void test02(): Assertion 'last_write_time(f.path) == time' failed.

>on arm*linux* and aarch64*linux* targets.


That test will fail for targets where _GLIBCXX_USE_UTIMENSAT is not
defined, as they use utime() instead which only supports second
granularity.

This should solve it, by only checking that the file times are within
one second of the expected value.

Tested x86_64-linux, committed to trunk.

Comments

Christophe Lyon Nov. 2, 2016, 9:09 a.m. UTC | #1 
On 27 October 2016 at 15:34, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 26/10/16 09:24 +0200, Christophe Lyon wrote:

>>

>> Hi Jonathan,

>>

>> On 25 October 2016 at 17:32, Jonathan Wakely <jwakely@redhat.com> wrote:

>>>

>>> Two more fixes for the filesystem TS, and improved tests.

>>>

>>>   Handle negative times in filesystem::last_write_time

>>>           * src/filesystem/ops.cc

>>>        (last_write_time(const path&, file_time_type, error_code&)):

>>> Handle

>>>        negative times correctly.

>>>        * testsuite/experimental/filesystem/operations/last_write_time.cc:

>>>        Test writing file times.

>>>

>>>    Fix error handling in copy_file and equivalent

>>>           * src/filesystem/ops.cc (do_copy_file): Report an error if

>>> source

>>> or

>>>        destination is not a regular file (LWG 2712).

>>>        (equivalent): Fix error handling and result when only one file

>>> exists.

>>>        * testsuite/experimental/filesystem/operations/copy.cc: Remove

>>> files

>>>        created by tests. Test copying directories.

>>>        * testsuite/experimental/filesystem/operations/copy_file.cc:

>>> Remove

>>>        files created by tests.

>>>        * testsuite/experimental/filesystem/operations/equivalent.cc: New.

>>>        * testsuite/experimental/filesystem/operations/is_empty.cc: New.

>>>        * testsuite/experimental/filesystem/operations/read_symlink.cc:

>>> Remove

>>>        file created by test.

>>>        * testsuite/experimental/filesystem/operations/remove_all.cc: New.

>>>        * testsuite/util/testsuite_fs.h (~scoped_file): Only try to remove

>>>        file if path is non-empty, to support removal by other means.

>>>

>>> Tested x86_64-linux, committed to trunk.

>>>

>>>

>> I can see failures in

>> experimental/filesystem/operations/last_write_time.cc after your

>> committed this patch:

>>

>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/testsuite/experimental/filesystem/operations/last_write_time.cc:127:

>> void test02(): Assertion 'last_write_time(f.path) == time' failed.

>> on arm*linux* and aarch64*linux* targets.

>

>

> That test will fail for targets where _GLIBCXX_USE_UTIMENSAT is not

> defined, as they use utime() instead which only supports second

> granularity.

>

> This should solve it, by only checking that the file times are within

> one second of the expected value.

>


Hi Jonathan,
Indeed your patch fixes the problem I reported.
Sorry for the delay, I was on holidays.

Thanks,

Christophe


>

> Tested x86_64-linux, committed to trunk.
Christophe Lyon Nov. 4, 2016, 2:31 p.m. UTC | #2 
On 2 November 2016 at 10:09, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> On 27 October 2016 at 15:34, Jonathan Wakely <jwakely@redhat.com> wrote:

>> On 26/10/16 09:24 +0200, Christophe Lyon wrote:

>>>

>>> Hi Jonathan,

>>>

>>> On 25 October 2016 at 17:32, Jonathan Wakely <jwakely@redhat.com> wrote:

>>>>

>>>> Two more fixes for the filesystem TS, and improved tests.

>>>>

>>>>   Handle negative times in filesystem::last_write_time

>>>>           * src/filesystem/ops.cc

>>>>        (last_write_time(const path&, file_time_type, error_code&)):

>>>> Handle

>>>>        negative times correctly.

>>>>        * testsuite/experimental/filesystem/operations/last_write_time.cc:

>>>>        Test writing file times.

>>>>

>>>>    Fix error handling in copy_file and equivalent

>>>>           * src/filesystem/ops.cc (do_copy_file): Report an error if

>>>> source

>>>> or

>>>>        destination is not a regular file (LWG 2712).

>>>>        (equivalent): Fix error handling and result when only one file

>>>> exists.

>>>>        * testsuite/experimental/filesystem/operations/copy.cc: Remove

>>>> files

>>>>        created by tests. Test copying directories.

>>>>        * testsuite/experimental/filesystem/operations/copy_file.cc:

>>>> Remove

>>>>        files created by tests.

>>>>        * testsuite/experimental/filesystem/operations/equivalent.cc: New.

>>>>        * testsuite/experimental/filesystem/operations/is_empty.cc: New.

>>>>        * testsuite/experimental/filesystem/operations/read_symlink.cc:

>>>> Remove

>>>>        file created by test.

>>>>        * testsuite/experimental/filesystem/operations/remove_all.cc: New.

>>>>        * testsuite/util/testsuite_fs.h (~scoped_file): Only try to remove

>>>>        file if path is non-empty, to support removal by other means.

>>>>

>>>> Tested x86_64-linux, committed to trunk.

>>>>

>>>>

>>> I can see failures in

>>> experimental/filesystem/operations/last_write_time.cc after your

>>> committed this patch:

>>>

>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/testsuite/experimental/filesystem/operations/last_write_time.cc:127:

>>> void test02(): Assertion 'last_write_time(f.path) == time' failed.

>>> on arm*linux* and aarch64*linux* targets.

>>

>>

>> That test will fail for targets where _GLIBCXX_USE_UTIMENSAT is not

>> defined, as they use utime() instead which only supports second

>> granularity.

>>

>> This should solve it, by only checking that the file times are within

>> one second of the expected value.

>>

>

> Hi Jonathan,

> Indeed your patch fixes the problem I reported.

> Sorry for the delay, I was on holidays.

>


.... but experimental/filesystem/iterators/recursive_directory_iterator.cc
now fails at execution on "old" arm targets (when forcing -march=armv5t).

Christophe


> Thanks,

>

> Christophe

>

>

>>

>> Tested x86_64-linux, committed to trunk.
diff mbox

Patch

commit 9b3f71e225ae45ccc1a5a78fee05d4ed9a969e8e
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Oct 27 11:48:00 2016 +0100

    Adjust precision of filesystem::last_write_time tests
    
    	* testsuite/experimental/filesystem/iterators/directory_iterator.cc:
    	Use end() function to get end iterator.
    	* testsuite/experimental/filesystem/iterators/pop.cc: Remove printf
    	statements that were present for debugging.
    	* testsuite/experimental/filesystem/iterators/
    	recursive_directory_iterator.cc: Use end() function to get end
    	iterator.
    	* testsuite/experimental/filesystem/operations/last_write_time.cc:
    	Only require file timestamps to be accurate to one second.

diff --git a/libstdc++-v3/testsuite/experimental/filesystem/iterators/directory_iterator.cc b/libstdc++-v3/testsuite/experimental/filesystem/iterators/directory_iterator.cc
index 5788700..d1f2c54 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/iterators/directory_iterator.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/iterators/directory_iterator.cc
@@ -34,14 +34,14 @@  test01()
   const auto p = __gnu_test::nonexistent_path();
   fs::directory_iterator iter(p, ec);
   VERIFY( ec );
-  VERIFY( iter == fs::directory_iterator() );
+  VERIFY( iter == end(iter) );
 
   // Test empty directory.
   create_directory(p, fs::current_path(), ec);
   VERIFY( !ec );
   iter = fs::directory_iterator(p, ec);
   VERIFY( !ec );
-  VERIFY( iter == fs::directory_iterator() );
+  VERIFY( iter == end(iter) );
 
   // Test non-empty directory.
   create_directory_symlink(p, p / "l", ec);
@@ -51,20 +51,20 @@  test01()
   VERIFY( iter != fs::directory_iterator() );
   VERIFY( iter->path() == p/"l" );
   ++iter;
-  VERIFY( iter == fs::directory_iterator() );
+  VERIFY( iter == end(iter) );
 
   // Test inaccessible directory.
   permissions(p, fs::perms::none, ec);
   VERIFY( !ec );
   iter = fs::directory_iterator(p, ec);
   VERIFY( ec );
-  VERIFY( iter == fs::directory_iterator() );
+  VERIFY( iter == end(iter) );
 
   // Test inaccessible directory, skipping permission denied.
   const auto opts = fs::directory_options::skip_permission_denied;
   iter = fs::directory_iterator(p, opts, ec);
   VERIFY( !ec );
-  VERIFY( iter == fs::directory_iterator() );
+  VERIFY( iter == end(iter) );
 
   permissions(p, fs::perms::owner_all, ec);
   remove_all(p, ec);
@@ -82,12 +82,12 @@  test02()
   // Test post-increment (libstdc++/71005)
   auto iter = fs::directory_iterator(p, ec);
   VERIFY( !ec );
-  VERIFY( iter != fs::directory_iterator() );
+  VERIFY( iter != end(iter) );
   const auto entry1 = *iter;
   const auto entry2 = *iter++;
   VERIFY( entry1 == entry2 );
   VERIFY( entry1.path() == p/"l" );
-  VERIFY( iter == fs::directory_iterator() );
+  VERIFY( iter == end(iter) );
 
   remove_all(p, ec);
 }
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/iterators/pop.cc b/libstdc++-v3/testsuite/experimental/filesystem/iterators/pop.cc
index d247ab4..9306c03 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/iterators/pop.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/iterators/pop.cc
@@ -84,14 +84,10 @@  test03()
     std::advance(dir, i);
     int expected_depth = i;
     VERIFY( dir.depth() == expected_depth );
-    __builtin_printf("%d %d %s\n", i, dir.depth(), dir->path().c_str());
     dir.pop(ec);
     VERIFY( !ec );
     if (dir != end(dir))
-    {
-    __builtin_printf("%d %d %s\n", i, dir.depth(), dir->path().c_str());
       VERIFY( dir.depth() == (expected_depth - 1) );
-    }
 
     dir = fs::recursive_directory_iterator(p);
     std::advance(dir, i);
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/iterators/recursive_directory_iterator.cc b/libstdc++-v3/testsuite/experimental/filesystem/iterators/recursive_directory_iterator.cc
index 79aa178..3dc7ba2 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/iterators/recursive_directory_iterator.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/iterators/recursive_directory_iterator.cc
@@ -34,39 +34,39 @@  test01()
   const auto p = __gnu_test::nonexistent_path();
   fs::recursive_directory_iterator iter(p, ec);
   VERIFY( ec );
-  VERIFY( iter == fs::recursive_directory_iterator() );
+  VERIFY( iter == end(iter) );
 
   // Test empty directory.
   create_directory(p, fs::current_path(), ec);
   VERIFY( !ec );
   iter = fs::recursive_directory_iterator(p, ec);
   VERIFY( !ec );
-  VERIFY( iter == fs::recursive_directory_iterator() );
+  VERIFY( iter == end(iter) );
 
   // Test non-empty directory.
   create_directories(p / "d1/d2");
   VERIFY( !ec );
   iter = fs::recursive_directory_iterator(p, ec);
   VERIFY( !ec );
-  VERIFY( iter != fs::recursive_directory_iterator() );
+  VERIFY( iter != end(iter) );
   VERIFY( iter->path() == p/"d1" );
   ++iter;
   VERIFY( iter->path() == p/"d1/d2" );
   ++iter;
-  VERIFY( iter == fs::recursive_directory_iterator() );
+  VERIFY( iter == end(iter) );
 
   // Test inaccessible directory.
   permissions(p, fs::perms::none, ec);
   VERIFY( !ec );
   iter = fs::recursive_directory_iterator(p, ec);
   VERIFY( ec );
-  VERIFY( iter == fs::recursive_directory_iterator() );
+  VERIFY( iter == end(iter) );
 
   // Test inaccessible directory, skipping permission denied.
   const auto opts = fs::directory_options::skip_permission_denied;
   iter = fs::recursive_directory_iterator(p, opts, ec);
   VERIFY( !ec );
-  VERIFY( iter == fs::recursive_directory_iterator() );
+  VERIFY( iter == end(iter) );
 
   // Test inaccessible sub-directory.
   permissions(p, fs::perms::owner_all, ec);
@@ -75,24 +75,24 @@  test01()
   VERIFY( !ec );
   iter = fs::recursive_directory_iterator(p, ec);
   VERIFY( !ec );
-  VERIFY( iter != fs::recursive_directory_iterator() );
+  VERIFY( iter != end(iter) );
   VERIFY( iter->path() == p/"d1" );
   ++iter;              // should recurse into d1
   VERIFY( iter->path() == p/"d1/d2" );
   iter.increment(ec);  // should fail to recurse into p/d1/d2
   VERIFY( ec );
-  VERIFY( iter == fs::recursive_directory_iterator() );
+  VERIFY( iter == end(iter) );
 
   // Test inaccessible sub-directory, skipping permission denied.
   iter = fs::recursive_directory_iterator(p, opts, ec);
   VERIFY( !ec );
-  VERIFY( iter != fs::recursive_directory_iterator() );
+  VERIFY( iter != end(iter) );
   VERIFY( iter->path() == p/"d1" );
   ++iter;              // should recurse into d1
   VERIFY( iter->path() == p/"d1/d2" );
   iter.increment(ec);  // should fail to recurse into p/d1/d2, so skip it
   VERIFY( !ec );
-  VERIFY( iter == fs::recursive_directory_iterator() );
+  VERIFY( iter == end(iter) );
 
   permissions(p/"d1/d2", fs::perms::owner_all, ec);
   remove_all(p, ec);
@@ -109,7 +109,7 @@  test02()
   // Test post-increment (libstdc++/71005)
   auto iter = fs::recursive_directory_iterator(p, ec);
   VERIFY( !ec );
-  VERIFY( iter != fs::recursive_directory_iterator() );
+  VERIFY( iter != end(iter) );
   const auto entry1 = *iter;
   const auto entry2 = *iter++;
   VERIFY( entry1 == entry2 );
@@ -118,7 +118,7 @@  test02()
   const auto entry4 = *iter++;
   VERIFY( entry3 == entry4 );
   VERIFY( entry3.path() == p/"d1/d2" );
-  VERIFY( iter == fs::recursive_directory_iterator() );
+  VERIFY( iter == end(iter) );
 
   remove_all(p, ec);
 }
@@ -145,7 +145,7 @@  test04()
 {
   // libstdc++/71004
   const fs::recursive_directory_iterator it;
-  VERIFY( it == fs::recursive_directory_iterator() );
+  VERIFY( it == end(it) );
 }
 
 void
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/last_write_time.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/last_write_time.cc
index 74be4f9..a60a25f 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/last_write_time.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/last_write_time.cc
@@ -32,13 +32,13 @@ 
 # include <utime.h>
 #endif
 
+using time_type = std::experimental::filesystem::file_time_type;
+
 void
 test01()
 {
   // read times
 
-  using time_type = std::experimental::filesystem::file_time_type;
-
   auto p = __gnu_test::nonexistent_path();
   std::error_code ec;
   time_type mtime = last_write_time(p, ec);
@@ -105,13 +105,19 @@  test01()
 #endif
 }
 
+bool approx_equal(time_type file_time, time_type expected)
+{
+  auto delta = expected - file_time;
+  if (delta < delta.zero())
+    delta = -delta;
+  return delta < std::chrono::seconds(1);
+}
+
 void
 test02()
 {
   // write times
 
-  using time_type = std::experimental::filesystem::file_time_type;
-
   __gnu_test::scoped_file f;
   std::error_code ec;
   time_type time;
@@ -119,27 +125,27 @@  test02()
   time = last_write_time(f.path);
   last_write_time(f.path, time, ec);
   VERIFY( !ec );
-  VERIFY( last_write_time(f.path) == time );
+  VERIFY( approx_equal(last_write_time(f.path), time) );
 
   time -= std::chrono::milliseconds(1000 * 60 * 10 + 15);
   last_write_time(f.path, time, ec);
   VERIFY( !ec );
-  VERIFY( last_write_time(f.path) == time );
+  VERIFY( approx_equal(last_write_time(f.path), time) );
 
   time += std::chrono::milliseconds(1000 * 60 * 20 + 15);
   last_write_time(f.path, time, ec);
   VERIFY( !ec );
-  VERIFY( last_write_time(f.path) == time );
+  VERIFY( approx_equal(last_write_time(f.path), time) );
 
   time = time_type();
   last_write_time(f.path, time, ec);
   VERIFY( !ec );
-  VERIFY( last_write_time(f.path) == time );
+  VERIFY( approx_equal(last_write_time(f.path), time) );
 
   time -= std::chrono::milliseconds(1000 * 60 * 10 + 15);
   last_write_time(f.path, time, ec);
   VERIFY( !ec );
-  VERIFY( last_write_time(f.path) == time );
+  VERIFY( approx_equal(last_write_time(f.path), time) );
 }
 
 int