Page MenuHomePhabricator

[libc++] [P1164] [C++20] Make fs::create_directory() error if there is already a non-directory.
ClosedPublic

Authored by curdeius on Dec 7 2020, 8:37 AM.

Details

Summary

Also mark LWG2935 and LWG3079 as complete.

Applied retroactively to previous standards too, as it's a DR.

Diff Detail

Event Timeline

curdeius created this revision.Dec 7 2020, 8:37 AM
curdeius requested review of this revision.Dec 7 2020, 8:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2020, 8:37 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
curdeius edited the summary of this revision. (Show Details)Dec 7 2020, 8:40 AM
curdeius added inline comments.
libcxx/src/filesystem/operations.cpp
858

Question to reviewers, what should we return, error_code corresponding to errno (mec) or another error code, e.g not_a_directory?

888

I'll remove this line in the next revision.

ldionne requested changes to this revision.Dec 7 2020, 3:26 PM

I'm not sure if it should be applied retroactively to C++17 too (as I've done currently).

Yes, we apply LWG resolutions retroactively to all standards.

Also, you'll need markup for some Apple system libraries that shipped Filesystem but don't contain that fix. They are not covered by CI right now but they will once I land https://reviews.llvm.org/D92794. I'll let you know and you can rebase on top of it -- it should be trivial to add the UNSUPPORTED annotations.

libcxx/docs/Cxx2aStatusIssuesStatus.csv
18

Isn't that "Nothing to do"?

86

Same here -- isn't that "Nothing to do"?

libcxx/src/filesystem/operations.cpp
858

I believe an error code corresponding to errno is the most sensible thing, since that's effectively surfacing the actual failure reported by mkdir to the user, which is what we want IMO. Also, the Standard gives us leeway here, it only seems to say that we need to report an error -- so either way appears to be conforming.

This revision now requires changes to proceed.Dec 7 2020, 3:26 PM
curdeius updated this revision to Diff 310095.Dec 7 2020, 11:38 PM
  • Mark LWG issues as Nothing To Do.
  • Remove TODOs.
curdeius marked 3 inline comments as done.Dec 7 2020, 11:40 PM

OK. I'll wait for D92794 to rebase and update.

libcxx/docs/Cxx2aStatusIssuesStatus.csv
18

Good catch. Updated accordingly.

86

Ditto.

libcxx/src/filesystem/operations.cpp
858

Ok. Thanks.

curdeius planned changes to this revision.Dec 7 2020, 11:40 PM
curdeius marked 3 inline comments as done.
curdeius updated this revision to Diff 310239.Dec 8 2020, 9:10 AM
  • Rebase.

@ldionne, I was expecting tests to fail, but somehow they are already listed as unsupported on macOS 10.14...

https://buildkite.com/llvm-project/libcxx-ci/builds/671#028852c7-7ffc-4d95-8455-62d0d4c6c05e/6-545

libc++ :: std/input.output/filesystems/fs.op.funcs/fs.op.create_directory/create_directory.pass.cpp
libc++ :: std/input.output/filesystems/fs.op.funcs/fs.op.create_directory/create_directory_with_attributes.pass.cpp

How is that?

Oh, I got my answer...
libcxx/test/std/input.output/filesystems/lit.local.cfg is:

if 'c++filesystem-disabled' in config.available_features:
  config.unsupported = True

But... haven't you said that filesystem landed in macOS 10.14?

And I see this in 10.14 build log:

llvm-lit: /tmp/buildkite-builds/mbp-2-local-1/llvm-project/libcxx-ci/libcxx/utils/libcxx/test/config.py:146: note: All available features: {'locale.fr_CA.ISO8859-1', 'modules-support', 'with_system_cxx_lib=macosx', 'libc++', 'apple-clang-12.0', 'thread-safety', 'long_tests', 'locale.fr_FR.UTF-8', 'fcoroutines-ts', '-faligned-allocation', 'locale.zh_CN.UTF-8', '-fsized-deallocation', 'libcxx-no-debug-mode', 'fdelayed-template-parsing', 'apple-clang-12.0.0', 'x86_64-apple', 'c++filesystem-disabled', '__dummy_use_system_cxx_lib', 'libcpp-no-concepts', 'locale.ru_RU.UTF-8', 'apple-clang', 'has-fobjc-arc', 'libcpp-abi-version=1', 'apple-clang-12', 'with_system_cxx_lib=x86_64-apple-macosx10.14', 'use_system_cxx_lib', 'target-x86_64', 'locale.en_US.UTF-8', 'with_system_cxx_lib=macosx10.14', 'locale.cs_CZ.ISO8859-2', 'objective-c++', 'diagnose-if-support', 'darwin', 'libcpp-has-thread-api-pthread', 'c++2a', 'has-fblocks'}

so, there is 'c++filesystem-disabled'.
Seems like a bug, nope?

Oh, I got my answer...
libcxx/test/std/input.output/filesystems/lit.local.cfg is:

if 'c++filesystem-disabled' in config.available_features:
  config.unsupported = True

But... haven't you said that filesystem landed in macOS 10.14?

I apologize for the confusion, I made a mistake: filesystem was introduced in macOS 10.15, not 10.14. Fixing in https://reviews.llvm.org/D92937.

curdeius updated this revision to Diff 310541.Dec 9 2020, 8:11 AM
  • Rebase to trigger CI on top of parent revision D92937.
curdeius updated this revision to Diff 310562.Dec 9 2020, 9:19 AM
  • Mark XFAIL on macosx10.15.
ldionne accepted this revision.Dec 9 2020, 9:57 AM

LGTM once CI passes!

This revision is now accepted and ready to land.Dec 9 2020, 9:57 AM
curdeius edited the summary of this revision. (Show Details)Dec 9 2020, 11:39 PM

Note that P1164 talks about both create_directory and create_directories, while this fix only covers create_directory.

Note that P1164 talks about both create_directory and create_directories, while this fix only covers create_directory.

Indeed, but it doesn't change the behaviour of create_directories per se because it calls create_directory for every part of the path.
(Actually it changes a phrase from the "Returns" clause that was actually implying a contradiction on what should really be returned, but libc++ was doing already the right thing.)

Anyway, I'll add a test for create_directories in a follow-up patch.

Note that P1164 talks about both create_directory and create_directories, while this fix only covers create_directory.

Indeed, but it doesn't change the behaviour of create_directories per se because it calls create_directory for every part of the path.

Ah, right.

After revisiting the issue I was, it was that while create_directories does report failures, it reports a different error code than expected - at least with the windows filesystem APIs. (But maybe the testcase I'm running, one picked from MS STL's testsuite, is overly strict on checking the specific error codes?) I'll comment inline to point out and explain the issue.

libcxx/src/filesystem/operations.cpp
844

If parent does exist, but isn't a directory, the check if (not exists(parent_st)) above won't trigger, so it won't hit the case of calling __create_directory("parent_which_is_file"), but will instead call __create_directory("parent_which_is_file/subdir"). That call doesn't return the error code corresponding to EEXIST, but gives an error corresponding to no_such_file_or_directory/ENOENT instead.

By extending the check above into if (not exists(parent_st)) { ... } else if (!is_directory(parent)) return err.report(errc::file_exists);, one would get the more expected error code here too.

curdeius marked an inline comment as done.Dec 11 2020, 12:53 AM
curdeius added inline comments.
libcxx/src/filesystem/operations.cpp
844

Moved discussion to D93026.

844

If parent does exist, but isn't a directory, the check if (not exists(parent_st)) above won't trigger, so it won't hit the case of calling __create_directory("parent_which_is_file"), but will instead call __create_directory("parent_which_is_file/subdir"). That call doesn't return the error code corresponding to EEXIST, but gives an error corresponding to no_such_file_or_directory/ENOENT instead.

By extending the check above into if (not exists(parent_st)) { ... } else if (!is_directory(parent)) return err.report(errc::file_exists);, one would get the more expected error code here too.

curdeius marked an inline comment as done.Dec 11 2020, 12:54 AM