From 7fd2dd851da3401a5c7352cad8c57ef0c0adac59 Mon Sep 17 00:00:00 2001 From: redi Date: Thu, 4 Jan 2018 22:58:59 +0000 Subject: [PATCH] PR libstdc++/83626 Don't throw for remove("") and remove_all("") PR libstdc++/83626 * src/filesystem/ops.cc (remove(const path&, error_code&))): Remove redundant call to ec.clear(). (remove_all(const path&, error_code&))): Do not return an error for non-existent paths. * src/filesystem/std-ops.cc: Likewise. * testsuite/27_io/filesystem/operations/remove.cc: New test. * testsuite/27_io/filesystem/operations/remove_all.cc: Fix expected results for non-existent paths. * testsuite/experimental/filesystem/operations/remove.cc: New test. * testsuite/experimental/filesystem/operations/remove_all.cc: Fix expected results for non-existent paths. git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@256269 138bc75d-0d04-0410-961f-82ee72b054a4 --- libstdc++-v3/ChangeLog | 13 ++++ libstdc++-v3/src/filesystem/ops.cc | 41 +++++++----- libstdc++-v3/src/filesystem/std-ops.cc | 33 +++++---- .../operations/{remove_all.cc => remove.cc} | 76 +++++++++++---------- .../27_io/filesystem/operations/remove_all.cc | 14 ++-- .../operations/{remove_all.cc => remove.cc} | 78 ++++++++++++---------- .../filesystem/operations/remove_all.cc | 14 ++-- 7 files changed, 158 insertions(+), 111 deletions(-) copy libstdc++-v3/testsuite/27_io/filesystem/operations/{remove_all.cc => remove.cc} (60%) copy libstdc++-v3/testsuite/experimental/filesystem/operations/{remove_all.cc => remove.cc} (59%) diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog index 83361d45188..82de1ff85d0 100644 --- a/libstdc++-v3/ChangeLog +++ b/libstdc++-v3/ChangeLog @@ -1,5 +1,18 @@ 2018-01-04 Jonathan Wakely + PR libstdc++/83626 + * src/filesystem/ops.cc (remove(const path&, error_code&))): Remove + redundant call to ec.clear(). + (remove_all(const path&, error_code&))): Do not return an error for + non-existent paths. + * src/filesystem/std-ops.cc: Likewise. + * testsuite/27_io/filesystem/operations/remove.cc: New test. + * testsuite/27_io/filesystem/operations/remove_all.cc: Fix expected + results for non-existent paths. + * testsuite/experimental/filesystem/operations/remove.cc: New test. + * testsuite/experimental/filesystem/operations/remove_all.cc: Fix + expected results for non-existent paths. + * include/bits/fs_ops.h (exists(const path&, error_code&))): Only check status_known once. * include/experimental/bits/fs_ops.h: Likewise. diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc index 56b1432036f..5a7cb599bb6 100644 --- a/libstdc++-v3/src/filesystem/ops.cc +++ b/libstdc++-v3/src/filesystem/ops.cc @@ -1009,7 +1009,7 @@ fs::remove(const path& p) { error_code ec; bool result = fs::remove(p, ec); - if (ec.value()) + if (ec) _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove", p, ec)); return result; } @@ -1017,17 +1017,21 @@ fs::remove(const path& p) bool fs::remove(const path& p, error_code& ec) noexcept { - if (exists(symlink_status(p, ec))) + const auto s = symlink_status(p, ec); + if (!status_known(s)) + return false; + if (s.type() == file_type::not_found) { - if (::remove(p.c_str()) == 0) - { - ec.clear(); - return true; - } - else - ec.assign(errno, std::generic_category()); + ec.clear(); + return false; // Nothing to do, not an error. } - return false; + if (::remove(p.c_str()) != 0) + { + ec.assign(errno, std::generic_category()); + return false; + } + ec.clear(); + return true; } @@ -1036,7 +1040,7 @@ fs::remove_all(const path& p) { error_code ec; bool result = remove_all(p, ec); - if (ec.value()) + if (ec) _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove all", p, ec)); return result; } @@ -1044,14 +1048,19 @@ fs::remove_all(const path& p) std::uintmax_t fs::remove_all(const path& p, error_code& ec) noexcept { - auto fs = symlink_status(p, ec); + const auto s = symlink_status(p, ec); + if (!status_known(s)) + return -1; + ec.clear(); uintmax_t count = 0; - if (ec.value() == 0 && fs.type() == file_type::directory) - for (directory_iterator d(p, ec), end; ec.value() == 0 && d != end; ++d) + if (s.type() == file_type::directory) + for (directory_iterator d(p, ec), end; !ec && d != end; ++d) count += fs::remove_all(d->path(), ec); - if (ec.value()) + if (!ec && fs::remove(p, ec)) + ++count; + if (ec) return -1; - return fs::remove(p, ec) ? ++count : -1; // fs:remove() calls ec.clear() + return count; } void diff --git a/libstdc++-v3/src/filesystem/std-ops.cc b/libstdc++-v3/src/filesystem/std-ops.cc index 8392f6fc835..6ff280a9c76 100644 --- a/libstdc++-v3/src/filesystem/std-ops.cc +++ b/libstdc++-v3/src/filesystem/std-ops.cc @@ -1266,17 +1266,21 @@ fs::remove(const path& p) bool fs::remove(const path& p, error_code& ec) noexcept { - if (exists(symlink_status(p, ec))) + const auto s = symlink_status(p, ec); + if (!status_known(s)) + return false; + if (s.type() == file_type::not_found) { - if (::remove(p.c_str()) == 0) - { - ec.clear(); - return true; - } - else - ec.assign(errno, std::generic_category()); + ec.clear(); + return false; // Nothing to do, not an error. } - return false; + if (::remove(p.c_str()) != 0) + { + ec.assign(errno, std::generic_category()); + return false; + } + ec.clear(); + return true; } @@ -1293,14 +1297,19 @@ fs::remove_all(const path& p) std::uintmax_t fs::remove_all(const path& p, error_code& ec) { - auto fs = symlink_status(p, ec); + const auto s = symlink_status(p, ec); + if (!status_known(s)) + return -1; + ec.clear(); uintmax_t count = 0; - if (!ec && fs.type() == file_type::directory) + if (s.type() == file_type::directory) for (directory_iterator d(p, ec), end; !ec && d != end; ++d) count += fs::remove_all(d->path(), ec); + if (!ec && fs::remove(p, ec)) + ++count; if (ec) return -1; - return fs::remove(p, ec) ? ++count : -1; // fs:remove() calls ec.clear() + return count; } void diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove.cc similarity index 60% copy from libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc copy to libstdc++-v3/testsuite/27_io/filesystem/operations/remove.cc index 9cdc86fe9c3..ef9f06d7300 100644 --- a/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc +++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2016-2018 Free Software Foundation, Inc. +// Copyright (C) 2018 Free Software Foundation, Inc. // // This file is part of the GNU ISO C++ Library. This library is free // software; you can redistribute it and/or modify it under the @@ -29,60 +29,68 @@ void test01() { std::error_code ec; - std::uintmax_t n; + const std::error_code bad_ec = make_error_code(std::errc::invalid_argument); + bool n; - n = fs::remove_all("", ec); - VERIFY( ec ); - VERIFY( n == std::uintmax_t(-1) ); + n = fs::remove("", ec); + VERIFY( !ec ); // This seems odd, but is what the standard requires. + VERIFY( !n ); auto p = __gnu_test::nonexistent_path(); - ec.clear(); - n = remove_all(p, ec); - VERIFY( ec ); - VERIFY( n == std::uintmax_t(-1) ); + ec = bad_ec; + n = remove(p, ec); + VERIFY( !ec ); + VERIFY( !n ); - const auto bad_ec = ec; auto link = __gnu_test::nonexistent_path(); create_symlink(p, link); // dangling symlink ec = bad_ec; - n = remove_all(link, ec); + n = remove(link, ec); VERIFY( !ec ); - VERIFY( n == 1 ); - VERIFY( !exists(symlink_status(link)) ); // DR 2721 + VERIFY( n ); + VERIFY( !exists(symlink_status(link)) ); __gnu_test::scoped_file f(p); create_symlink(p, link); ec = bad_ec; - n = remove_all(link, ec); + n = remove(link, ec); VERIFY( !ec ); - VERIFY( n == 1 ); + VERIFY( n ); VERIFY( !exists(symlink_status(link)) ); // The symlink is removed, but VERIFY( exists(p) ); // its target is not. - auto dir = __gnu_test::nonexistent_path(); - create_directories(dir/"a/b/c"); ec = bad_ec; - n = remove_all(dir/"a", ec); + n = remove(p, ec); VERIFY( !ec ); - VERIFY( n == 3 ); - VERIFY( exists(dir) ); - VERIFY( !exists(dir/"a") ); - - create_directories(dir/"a/b/c"); - __gnu_test::scoped_file a1(dir/"a/1"); - __gnu_test::scoped_file a2(dir/"a/2"); - __gnu_test::scoped_file b1(dir/"a/b/1"); - __gnu_test::scoped_file b2(dir/"a/b/2"); + VERIFY( n ); + VERIFY( !exists(symlink_status(p)) ); + + const auto dir = __gnu_test::nonexistent_path(); + create_directories(dir/"a/b"); + ec.clear(); + n = remove(dir/"a", ec); + VERIFY( ec ); + VERIFY( !n ); + VERIFY( exists(dir/"a/b") ); + + permissions(dir, fs::perms::none, ec); + if (!ec) + { + ec.clear(); + n = remove(dir/"a/b", ec); + VERIFY( ec ); + VERIFY( !n ); + permissions(dir, fs::perms::owner_all, ec); + } + ec = bad_ec; - n = remove_all(dir, ec); + n = remove(dir/"a/b", ec); VERIFY( !ec ); - VERIFY( n == 8 ); - VERIFY( !exists(dir) ); + VERIFY( n ); + VERIFY( !exists(dir/"a/b") ); - a1.path.clear(); - a2.path.clear(); - b1.path.clear(); - b2.path.clear(); + remove(dir/"a", ec); + remove(dir, ec); } int diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc index 9cdc86fe9c3..8ee5c4d39c1 100644 --- a/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc +++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc @@ -29,19 +29,19 @@ void test01() { std::error_code ec; + const std::error_code bad_ec = make_error_code(std::errc::invalid_argument); std::uintmax_t n; n = fs::remove_all("", ec); - VERIFY( ec ); - VERIFY( n == std::uintmax_t(-1) ); + VERIFY( !ec ); // This seems odd, but is what the standard requires. + VERIFY( n == 0 ); auto p = __gnu_test::nonexistent_path(); - ec.clear(); + ec = bad_ec; n = remove_all(p, ec); - VERIFY( ec ); - VERIFY( n == std::uintmax_t(-1) ); + VERIFY( !ec ); + VERIFY( n == 0 ); - const auto bad_ec = ec; auto link = __gnu_test::nonexistent_path(); create_symlink(p, link); // dangling symlink ec = bad_ec; @@ -59,7 +59,7 @@ test01() VERIFY( !exists(symlink_status(link)) ); // The symlink is removed, but VERIFY( exists(p) ); // its target is not. - auto dir = __gnu_test::nonexistent_path(); + const auto dir = __gnu_test::nonexistent_path(); create_directories(dir/"a/b/c"); ec = bad_ec; n = remove_all(dir/"a", ec); diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove.cc similarity index 59% copy from libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc copy to libstdc++-v3/testsuite/experimental/filesystem/operations/remove.cc index 983998d78fa..8d1801099a2 100644 --- a/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc +++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2016-2018 Free Software Foundation, Inc. +// Copyright (C) 2018 Free Software Foundation, Inc. // // This file is part of the GNU ISO C++ Library. This library is free // software; you can redistribute it and/or modify it under the @@ -19,7 +19,7 @@ // { dg-do run { target c++11 } } // { dg-require-filesystem-ts "" } -#include +#include #include #include @@ -29,60 +29,68 @@ void test01() { std::error_code ec; - std::uintmax_t n; + const std::error_code bad_ec = make_error_code(std::errc::invalid_argument); + bool n; - n = fs::remove_all("", ec); - VERIFY( ec ); - VERIFY( n == std::uintmax_t(-1) ); + n = fs::remove("", ec); + VERIFY( !ec ); // This seems odd, but is what the standard requires. + VERIFY( !n ); auto p = __gnu_test::nonexistent_path(); - ec.clear(); - n = remove_all(p, ec); - VERIFY( ec ); - VERIFY( n == std::uintmax_t(-1) ); + ec = bad_ec; + n = remove(p, ec); + VERIFY( !ec ); + VERIFY( !n ); - const auto bad_ec = ec; auto link = __gnu_test::nonexistent_path(); create_symlink(p, link); // dangling symlink ec = bad_ec; - n = remove_all(link, ec); + n = remove(link, ec); VERIFY( !ec ); - VERIFY( n == 1 ); - VERIFY( !exists(symlink_status(link)) ); // DR 2721 + VERIFY( n ); + VERIFY( !exists(symlink_status(link)) ); __gnu_test::scoped_file f(p); create_symlink(p, link); ec = bad_ec; - n = remove_all(link, ec); + n = remove(link, ec); VERIFY( !ec ); - VERIFY( n == 1 ); + VERIFY( n ); VERIFY( !exists(symlink_status(link)) ); // The symlink is removed, but VERIFY( exists(p) ); // its target is not. - auto dir = __gnu_test::nonexistent_path(); - create_directories(dir/"a/b/c"); ec = bad_ec; - n = remove_all(dir/"a", ec); + n = remove(p, ec); VERIFY( !ec ); - VERIFY( n == 3 ); - VERIFY( exists(dir) ); - VERIFY( !exists(dir/"a") ); - - create_directories(dir/"a/b/c"); - __gnu_test::scoped_file a1(dir/"a/1"); - __gnu_test::scoped_file a2(dir/"a/2"); - __gnu_test::scoped_file b1(dir/"a/b/1"); - __gnu_test::scoped_file b2(dir/"a/b/2"); + VERIFY( n ); + VERIFY( !exists(symlink_status(p)) ); + + const auto dir = __gnu_test::nonexistent_path(); + create_directories(dir/"a/b"); + ec.clear(); + n = remove(dir/"a", ec); + VERIFY( ec ); + VERIFY( !n ); + VERIFY( exists(dir/"a/b") ); + + permissions(dir, fs::perms::none, ec); + if (!ec) + { + ec.clear(); + n = remove(dir/"a/b", ec); + VERIFY( ec ); + VERIFY( !n ); + permissions(dir, fs::perms::owner_all, ec); + } + ec = bad_ec; - n = remove_all(dir, ec); + n = remove(dir/"a/b", ec); VERIFY( !ec ); - VERIFY( n == 8 ); - VERIFY( !exists(dir) ); + VERIFY( n ); + VERIFY( !exists(dir/"a/b") ); - a1.path.clear(); - a2.path.clear(); - b1.path.clear(); - b2.path.clear(); + remove(dir/"a", ec); + remove(dir, ec); } int diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc index 983998d78fa..fa146b4317c 100644 --- a/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc +++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc @@ -29,19 +29,19 @@ void test01() { std::error_code ec; + const std::error_code bad_ec = make_error_code(std::errc::invalid_argument); std::uintmax_t n; n = fs::remove_all("", ec); - VERIFY( ec ); - VERIFY( n == std::uintmax_t(-1) ); + VERIFY( !ec ); // This seems odd, but is what the TS requires. + VERIFY( n == 0 ); auto p = __gnu_test::nonexistent_path(); - ec.clear(); + ec = bad_ec; n = remove_all(p, ec); - VERIFY( ec ); - VERIFY( n == std::uintmax_t(-1) ); + VERIFY( !ec ); + VERIFY( n == 0 ); - const auto bad_ec = ec; auto link = __gnu_test::nonexistent_path(); create_symlink(p, link); // dangling symlink ec = bad_ec; @@ -59,7 +59,7 @@ test01() VERIFY( !exists(symlink_status(link)) ); // The symlink is removed, but VERIFY( exists(p) ); // its target is not. - auto dir = __gnu_test::nonexistent_path(); + const auto dir = __gnu_test::nonexistent_path(); create_directories(dir/"a/b/c"); ec = bad_ec; n = remove_all(dir/"a", ec); -- 2.11.4.GIT