<filesystem>: directory_entry(const path&, error_code&) should return error code

[Arzaghi]

Fixes #232

I've tried to follow [fs.dir.entry.cons] of the Standard document. I think there were some test cases against the Standard definition in the tests, so I have modified them. Besides the original issue, I thought the functions under [fs.dir.entry.mods] need the same fix as well.

[Arzaghi]

@StephanTLavavej

Specifically, it is strange that the non-error_code construction can succeed, while the error_code construction fails and produces a different state. The general behavior of should be that the exception interface and error_code interface behave identically, except for how they report errors.

I definitely agree with you. So, Because we have several tests here, here and here that expect constructing a directory_entry using nonexistent path not to fail. Then the error_code constructor should behave identically(i.e: set error code = 0)
All these tests mean constructors should not fail for nonexistent paths.

Then the standard document explicitly says [fs.dir.entry.cons]:

explicit directory_entry(const filesystem::path& p);
directory_entry(const filesystem::path& p, error_code& ec);

1 Effects: Calls refresh() or refresh(ec), respectively.
2 Postconditions: path() == p if no error occurs, otherwise path() == filesystem::path()

So we can deduce that both refresh() and refresh(ec) should not fail for nonexistent paths too. However, this test line was against the deduction. So I remove it!
My last commit applies the above deduction.

This behavior appears to be correct. However, both Clang and GCC implementations do not follow this deduction!
Also, this commit of my PR behaves as libc++.

[strega-nil-ms]

Other than those minor nits, I'm in favor of this change; the discussion in #232 implies to me that this is the correct way to go; having refresh() and directory_entry(const filesystem::path&) having different behavior is a bug.

The choice to make refresh() on a non-existent file succeed is, imo, the correct behavior, especially given that LLVM's libc++ makes exactly the same choice here:

  _LIBCPP_INLINE_VISIBILITY
  void __refresh(error_code* __ec = nullptr) {
    __handle_error("in directory_entry::refresh", __ec, __do_refresh(),
                   /*allow_dne*/ true);
  }

[BillyONeal]

The choice to make refresh() on a non-existent file succeed is, imo, the correct behavior

That seems reasonable to me.

especially given that LLVM's libc++ makes exactly the same choice here:

FWIW I don't think "what libc++ does" is an arbiter of correct behavior. Not only is caching behavior intentionally not specified in the standard, what we cache is substantially different. The attributes they cache aren't different in character for existing files vs. nonexisting files which is not true for us.

=================================

Don't forget to consider the other effect this does: it makes directory_entry(const path&) fail for things that are not file-not-found, where previously it was non failing. (That is, after all, what #232 is talking about) You may want to specifically consider cases like broken symlinks or symlinks of the wrong directory-ness.

(I'm intentionally not making further statements on whether to take the change or not given that I'm no longer a maintainer and haven't been looking at this closely for a long time, only pointing out things to consider :) )

[StephanTLavavej]

After catching up on all of the discussion here and in #232, checking the Standard, and checking this PR's behavior, I believe that this change is correct and avoids regressing any previous fixes. I pushed a very small addition to the test coverage.

Apologies for my exceptionally long review delay here! I've moved this to Final Review so Nicole can check the commits after her previous review - then we should be able to merge this next week.

[StephanTLavavej]

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

[StephanTLavavej]

Thanks again for investigating and fixing this bug in the C++17 filesystem library! 📂 🐞 😻

[e4lam]

Thank you all!