[libcxx-commits] [PATCH] D61915: [libcxx] change dylib- XFAILs to UNSUPPORTED

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D61915#1503571 <https://reviews.llvm.org/D61915#1503571>, @bcain wrote:

> In D61915#1503525 <https://reviews.llvm.org/D61915#1503525>, @ldionne wrote:
>
> > In D61915#1503235 <https://reviews.llvm.org/D61915#1503235>, @bcain wrote:
> >
> > > Maybe the presence of the filesystem header is enough to get some of these tests to pass.  [...]
> >
> >
> > Yes, I think this is exactly what's happening, and this is what I wanted to understand. I think the best solution would be for `LIBCXX_ENABLE_FILESYSTEM` to actually control whether filesystem is enabled in the library, however this includes:
> >
> > - whether we build support for filesystem in the dylib (that's the only thing we already do)
> > - whether the `<filesystem>` header is provided
> > - whether we provide operations like `std::ifstream::open()` on `std::filesystem::path` & friends
> >
> >   I think if we had that, then we could rename the lit feature from `dylib-has-no-filesystem` to just `has-no-filesystem`, and that feature would be enabled whenever:
> > - we are testing back-deployment to a platform that doesn't have dylib support for filesystem (and has availability markup that makes it invalid to use stuff like `std::ifstream::open(std::filesystem::path)`)
> > - we are testing libc++ trunk without filesystem support (your use case)
> >
> >   Is this something you'd be willing to investigate? I think having a good example of how to disable complete features from the library would be useful for other things as well, so this is something we need to do at some point. For example, our story for things like `LIBCXX_ENABLE_STDIN` and `LIBCXX_ENABLE_STDOUT` is similarly broken.
>
>
> Sure, I will take a look.  I tried whipping up a quick patch but I wasn't able to clean up all of the references from std::*fstream (though I think I figured out chrono).
>
> Also, I based that quick patch on the handling for `LIBCXX_ENABLE_THREADS` / `_LIBCPP_HAS_NO_THREADS`.  In that case, `<thread>` is still provided but hits an `#error` when included.  Does that suffice for what you think `<filesystem>` should do or should we really suppress the installation?


I think an `#error` is OK for now, but in the long term we'll want to make this friendlier to `__has_include`.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D61915