[libcxx-commits] [PATCH] D156585: [libc++][print] Make `<print>` tests require file system support.

Konstantin Varlamov via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Jul 31 14:40:47 PDT 2023


var-const marked 2 inline comments as done.
var-const added a comment.

In D156585#4544282 <https://reviews.llvm.org/D156585#4544282>, @Mordante wrote:

> I'm not against this patch, but I would like to investigate a bit further. When `FILE` is not available we will run into issues building the dylib. `src/print.cpp` includes `<print>` which has `FILE` in its interface. This works now because we build the dylib with C++20, but I expect it will fail when we switch to C++23. I have another patch for `print(ostream&...)` this patch  uses `FILE` in the dylib, so that is another potential issue before we switch to C++23. Can you test whether changing building the dylib with C++23 works on the platform in question?
>
> I wonder whether `no-filesystem` is intended to mean no `cstdio`. If this patch is not urgent I would like to discuss it when Louis is back. If it's urgent we can finish the patch and discuss afterwards.

It's somewhat urgent -- this patch is necessary to fix our internal build.

If I switch to C++23, I get the `FILE`-related errors while compiling `print.cpp` like you described.

I was trying to follow the approach previously laid out in https://reviews.llvm.org/D152168 for this patch. D152168 <https://reviews.llvm.org/D152168> combined the check for support for `<filesystem>` and `<fstream>` into a single macro, which makes me feel we shouldn't introduce a separate macro for `<cstdio>`. There are platforms that don't provide `FILE`, so I believe we need some macro to express that, the only question is whether we want to combine it with `no-filesystem` or not.



================
Comment at: libcxx/test/std/input.output/iostream.format/print.fun/lit.local.cfg:1
+if "no-filesystem" in config.available_features:
+    config.unsupported = True
----------------
Mordante wrote:
> Why do we need this? AFAIK we normally don't do this. Typically we disable all tests manually.
This approach is used heavily in `test/std/input.output`, mostly to disable tests if localization is not available (there are 10 existing `lit.local.cfg` files in that directory). I did it this way mostly for consistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156585



More information about the libcxx-commits mailing list