This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Make LIBCXX_ENABLE_FILESYSTEM fully consistent
ClosedPublic

Authored by ldionne on Jan 18 2021, 10:12 AM.

Details

Reviewers
rnk
mstorsjo
Group Reviewers
Restricted Project
Commits
rG933518fff82c: [libc++] Make LIBCXX_ENABLE_FILESYSTEM fully consistent
Summary

Previously, LIBCXX_ENABLE_FILESYSTEM controlled only whether the filesystem
support was compiled into libc++'s library. This commit promotes the
setting to a first-class option like LIBCXX_ENABLE_LOCALIZATION, where
the whole library is aware of the setting and features that depend on
<filesystem> won't be provided at all. The test suite is also properly
annotated such that tests that depend on <filesystem> are disabled when
the library doesn't support it.

This is an alternative to https://llvm.org/D94824, but also an improvement
along the lines of LIBCXX_ENABLE_LOCALIZATION that I had been wanting to
make for a while.

Diff Detail

Event Timeline

ldionne created this revision.Jan 18 2021, 10:12 AM
ldionne requested review of this revision.Jan 18 2021, 10:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2021, 10:12 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

FWIW, +1 from me on this one. I've earlier run into cases where code checks for whether <filesystem> is available and works, where it has accidentally seemed to work as long as the test only used features provided entirely by the header. See e.g. https://codereview.qt-project.org/c/qt/qtbase/+/295469 for such a case.

ldionne updated this revision to Diff 317564.Jan 19 2021, 7:33 AM

Fix modules tests.

rnk added a comment.Jan 19 2021, 9:55 AM

Thanks!

libcxx/include/filesystem
255

What do you think of allowing users to feature test with __has_include(<filesystem>)? This could be implemented as part of the installation rules for include/*, but that adds some complexity, and it will not work when using libc++ from a build directory. It seems nice-to-have to me, but not super important.

ldionne accepted this revision as: Restricted Project.Jan 19 2021, 11:12 AM
ldionne added inline comments.
libcxx/include/filesystem
255

Using __has_include to test whether features are enabled is an anti-pattern, and I think we shouldn't do anything towards making it "better". For example, people try to check whether filesystem is enabled in C++17 by checking for #if __has_include(<filesystem>), but that never worked (and couldn't work unless we installed different headers for different dialects). This here is the same thing IMO.

The right way to test is to use feature-test macros. I think we should instead make sure that feature-test macros respect _LIBCPP_HAS_NO_FILESYSTEM_LIBRARY, so that you can check for __cpp_lib_filesystem and have it return the right answer.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 19 2021, 11:16 AM
This revision was automatically updated to reflect the committed changes.