This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by TyanNN on Jan 8 2018, 11:48 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

TyanNN created this revision.Jan 8 2018, 11:48 AM
TyanNN edited the summary of this revision. (Show Details)Jan 8 2018, 11:51 AM
TyanNN updated this revision to Diff 129115.Jan 9 2018, 10:01 AM
TyanNN retitled this revision from [libc++] Fix PR#35780 - make std::experimental::filesystem::remove return false if the file doesn't exist to [libc++] Fix PR#35780 - make std::experimental::filesystem::remove and remove_all return false or 0 if the file doesn't exist.
TyanNN edited the summary of this revision. (Show Details)

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

mclow.lists added inline comments.Jan 9 2018, 10:53 AM
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 added inline comments.Jan 9 2018, 11:10 AM
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 updated this revision to Diff 129280.Jan 10 2018, 8:47 AM

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

TyanNN added inline comments.Jan 10 2018, 8:50 AM
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

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 added inline comments.Jan 10 2018, 4:36 PM
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 marked 2 inline comments as done.Jan 10 2018, 8:23 PM
TyanNN added inline comments.
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 added inline comments.Jan 10 2018, 9:17 PM
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 added inline comments.Jan 11 2018, 7:28 AM
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 updated this revision to Diff 129453.Jan 11 2018, 8:01 AM

Fix tests

TyanNN added inline comments.Jan 11 2018, 8:02 AM
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 accepted this revision.Jan 11 2018, 8:38 AM

LGTM.

Do you need me to commit this?

This revision is now accepted and ready to land.Jan 11 2018, 8:38 AM

LGTM.

Do you need me to commit this?

Nope, i can do it myself

This revision was automatically updated to reflect the committed changes.
EricWF added inline comments.Jan 11 2018, 2:09 PM
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 added inline comments.Jan 11 2018, 8:31 PM
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.

TyanNN marked 2 inline comments as done.Jan 11 2018, 9:03 PM