[libc++] Fix PR#35780 - make std::experimental::filesystem::remove and remove_all return false or 0 if the file doesn't exist

[TyanNN]

Summary

Previously both threw an error if the file didn't exist.

PR#35780

[TyanNN]

Fix remove_all too. Use std error codes instead of those from C

[mclow.lists]

src/experimental/filesystem/operations.cpp

666 ↗

(On Diff #129115)

I don't think that this is correct.

In the case where ec == nullptr AND the error is not "no such file", this will fail to throw.

I think you just want this test to be:

if (errno != ENOENT)

[mclow.lists]

src/experimental/filesystem/operations.cpp

699 ↗

(On Diff #129115)

I don't think that this is quite right, either.

In the case where you call remove_all("doesNotExist", &ec), this code will return 0, but will fail to clear ec.

Maybe this (untested) version instead:

if (ec) ec->clear();
auto count = remove_all_impl(p, mec);
if (mec) {
  if (mec == errc::no_such_file_or_directory)
    return 0;
  else {
    set_or_throw(mec, ec, "remove_all", p);
    return static_cast<std::uintmax_t>(-1);
  }
}
return count;

[TyanNN]

Fix remove and remove_all error resetting. Move test to the appropriate files and fix old tests.

[TyanNN]

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

67 ↗

(On Diff #129280)

This error is quite strange: it is not inherited from std::exception, one can only really find our the error with std::current_exception, and it happens when you use the path in fs::remove and fs::remove_all. Any idea about this? @mclow.lists

[mclow.lists]

I like the fact that you've removed the extra test that you've added.
However, I think that modifying the tests as you've done is more work than is needed.
I *suspect* that all you need to do is to move a couple of test cases from the "calls which should fail" to the "calls that should succeed" list (and adjust for the fact that the calls return false even though they succeed)

For remove, that would be path("dne") and path("") . Just FYI - I believe that 'dne' is Eric's shorthand for "does not exist".

Something like this:

TEST_CASE(basic_remove_test)
{
    scoped_test_env env;
    const path dne = env.make_env_path("dne");
    const path link = env.create_symlink(dne, "link");
    const path nested_link = env.make_env_path("nested_link");
    create_symlink(link, nested_link);
    const path testCases1[] = {
        env.create_file("file", 42),
        env.create_dir("empty_dir"),
        nested_link,
        link
    };
    for (auto& p : testCases1) {
        std::error_code ec = std::make_error_code(std::errc::address_in_use);
        TEST_CHECK(remove(p, ec));
        TEST_CHECK(!ec);
        TEST_CHECK(!exists(symlink_status(p)));
    }

//  This is https://bugs.llvm.org/show_bug.cgi?id=35780
    const path testCases2[] = {
        env.make_env_path("dne"),
        ""
    };
    for (auto& p : testCases2) {
        std::error_code ec = std::make_error_code(std::errc::address_in_use);
        TEST_CHECK(!remove(p, ec));
        TEST_CHECK(!ec);
        TEST_CHECK(!exists(symlink_status(p)));
    }
}

[mclow.lists]

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

67 ↗

(On Diff #129280)

I'm not seeing this error on my system (nor are any of the bots reporting it AFAIK).

That looks like "uncaught_exceptions is not yet implemented", and that comes from the ABI library.

What are you using for an ABI library? Is it libc++abi?

[TyanNN]

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

67 ↗

(On Diff #129280)

Yes, I use libc++abi from trunk that I've built in llvm tree with libc++. I don't have a system wide installation of libc++abi so that must be it. Does uncommenting the line really work on your system? For the tests report an uncaught exception and fail.

[mclow.lists]

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

67 ↗

(On Diff #129280)

It's definitely not commented out on my system.
(or on the trunk)

[mclow.lists]

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

67 ↗

(On Diff #129280)

I see the message "uncaught_exceptions not yet implemented" in src/support/runtime/exception_fallback.ipp - but only in an #ifdef block for __EMSCRIPTEN__

[TyanNN]

Fix tests

[TyanNN]

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

67 ↗

(On Diff #129280)

Well, it somehow got fixed after moving tests for nonexistant files into a separate loop, so that's good.

[mclow.lists]

LGTM.

Do you need me to commit this?

[TyanNN]

In D41830#973419, @mclow.lists wrote:

LGTM.

Do you need me to commit this?

Nope, i can do it myself

[EricWF]

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

86

This test is incorrect. ec isn't passed to the function.

[TyanNN]

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

86

Indeed it isn't. I will fix it later today and commit.

libcxx/trunk/src/experimental/filesystem/operations.cpp

path __read_symlink(const path& p, std::error_code *ec) {
     if (ec) ec->clear();
     buff[ret] = 0;
     return {buff};
 }
 
 
 bool __remove(const path& p, std::error_code *ec) {
     if (ec) ec->clear();
+
     if (::remove(p.c_str()) == -1) {
+        if (errno != ENOENT)
             set_or_throw(ec, "remove", p);
         return false;
     }
     return true;
 }
 
 namespace {
 
 std::uintmax_t remove_all_impl(path const & p, std::error_code& ec)
std::uintmax_t remove_all_impl(path const & p, std::error_code& ec)
     }
     if (!__remove(p, &ec)) return npos;
     return count;
 }
 
 } // end namespace
 
 std::uintmax_t __remove_all(const path& p, std::error_code *ec) {
+    if (ec) ec->clear();
+
     std::error_code mec;
     auto count = remove_all_impl(p, mec);
     if (mec) {
+        if (mec == errc::no_such_file_or_directory) {
+            return 0;
+        } else {
             set_or_throw(mec, ec, "remove_all", p);
             return static_cast<std::uintmax_t>(-1);
         }
-    if (ec) ec->clear();
+    }
     return count;
 }
 
 void __rename(const path& from, const path& to, std::error_code *ec) {
     if (::rename(from.c_str(), to.c_str()) == -1)
         set_or_throw(ec, "rename", from, to);
     else if (ec)
         ec->clear();

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

#endif
     };
     scoped_test_env env;
     const path non_empty_dir = env.create_dir("dir");
     env.create_file(non_empty_dir / "file1", 42);
     const path bad_perms_dir = env.create_dir("bad_dir");
     const path file_in_bad_dir = env.create_file(bad_perms_dir / "file", 42);
     permissions(bad_perms_dir, perms::none);
     const path testCases[] = {
-        "",
-        env.make_env_path("dne"),
         non_empty_dir,
         file_in_bad_dir,
     };
     for (auto& p : testCases) {
         std::error_code ec;
+
         TEST_CHECK(!fs::remove(p, ec));
         TEST_CHECK(ec);
         TEST_CHECK(checkThrow(p, ec));
     }
+
+    // PR#35780
+    const path testCasesNonexistant[] = {
+        "",
+        env.make_env_path("dne")
+    };
+
+    for (auto& p : testCasesNonexistant) {
+        std::error_code ec;
+
+        TEST_CHECK(!fs::remove(p, ec));
+        TEST_CHECK(!ec);
+    }
 }
 
 TEST_CASE(basic_remove_test)
 {
     scoped_test_env env;
     const path dne = env.make_env_path("dne");
     const path link = env.create_symlink(dne, "link");
     const path nested_link = env.make_env_path("nested_link");

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

#endif
     env.create_file(non_empty_dir / "file1", 42);
     const path bad_perms_dir = env.create_dir("bad_dir");
     const path file_in_bad_dir = env.create_file(bad_perms_dir / "file", 42);
     permissions(bad_perms_dir, perms::none);
     const path bad_perms_file = env.create_file("file2", 42);
     permissions(bad_perms_file, perms::none);
 
     const path testCases[] = {
-        env.make_env_path("dne"),
         file_in_bad_dir
     };
     const auto BadRet = static_cast<std::uintmax_t>(-1);
     for (auto& p : testCases) {
         std::error_code ec;
+
         TEST_CHECK(fs::remove_all(p, ec) == BadRet);
         TEST_CHECK(ec);
         TEST_CHECK(checkThrow(p, ec));
     }
+
+    // PR#35780
+    const path testCasesNonexistant[] = {
+        "",
+        env.make_env_path("dne")
+    };
+    for (auto &p : testCasesNonexistant) {
+        std::error_code ec;
+
+        TEST_CHECK(fs::remove_all(p) == 0);
+        TEST_CHECK(!ec);
+    }
 }
 
 TEST_CASE(basic_remove_all_test)
 {
     scoped_test_env env;
     const path dne = env.make_env_path("dne");
     const path link = env.create_symlink(dne, "link");
     const path nested_link = env.make_env_path("nested_link");