[libcxx-commits] [PATCH] D94921: [libc++] Make LIBCXX_ENABLE_FILESYSTEM fully consistent

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision as: libc++.
ldionne added inline comments.


================
Comment at: libcxx/include/filesystem:255
+#if defined(_LIBCPP_HAS_NO_FILESYSTEM_LIBRARY)
+# error "The Filesystem library is not supported by this configuration of libc++"
+#endif
----------------
rnk wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94921/new/

https://reviews.llvm.org/D94921