[libc++] Don't use copy_file_impl_fstream when not declared - #214706
[libc++] Don't use copy_file_impl_fstream when not declared#214706AnthonyLatsis wants to merge 1 commit into
copy_file_impl_fstream when not declared#214706Conversation
|
@llvm/pr-subscribers-libcxx Author: Anthony Latsis (AnthonyLatsis) Changescopy_file_impl_fstream is only defined under Report Assisted-by: Claude Code Full diff: https://github.com/llvm/llvm-project/pull/214706.diff 1 Files Affected:
diff --git a/libcxx/src/filesystem/operations.cpp b/libcxx/src/filesystem/operations.cpp
index 3ff62c102d0ad..7099cd2c75ab6 100644
--- a/libcxx/src/filesystem/operations.cpp
+++ b/libcxx/src/filesystem/operations.cpp
@@ -362,7 +362,13 @@ bool copy_file_impl(FileDescriptor& read_fd, FileDescriptor& write_fd, error_cod
}
#elif defined(_LIBCPP_FILESYSTEM_USE_FSTREAM)
bool copy_file_impl(FileDescriptor& read_fd, FileDescriptor& write_fd, error_code& ec) {
+# if defined(_LIBCPP_FILESYSTEM_NEED_FSTREAM)
return copy_file_impl_fstream(read_fd, write_fd, ec);
+# else
+ // iostreams are unavailable in the no-locale build, just fail.
+ ec.assign(EINVAL, std::system_category());
+ return false;
+# endif
}
#else
# error "Unknown implementation for copy_file_impl"
|
424830c to
d14b460
Compare
`copy_file_impl_fstream` is declared and defined only under `_LIBCPP_FILESYSTEM_NEED_FSTREAM` (which additionally requires `_LIBCPP_HAS_LOCALIZATION`), and then used in a `copy_file_impl` variant under a disjoint set of conditions. This causes any build that disables localization and elects this variant of `copy_file_impl`, as is currently the case when targeting wasi, to fail with a generic compiler error: ``` error: use of undeclared identifier 'copy_file_impl_fstream' ``` Instead, since this particular `copy_file_impl` implementation merely forwards to `copy_file_impl_fstream`, tweak its gating condition such that the aforementioned configuration hits the dedicated fallback branch, which offers a somewhat clearer error message: ``` \# error "Unknown implementation for copy_file_impl" ```
d14b460 to
4283f49
Compare
copy_file_impl_fstreamcopy_file_impl_fstream when not declared
ldionne
left a comment
There was a problem hiding this comment.
I find the #elif defined(_LIBCPP_FILESYSTEM_USE_FSTREAM) && defined(_LIBCPP_FILESYSTEM_NEED_FSTREAM) conditional a bit odd, but I think that's because of the name _LIBCPP_FILESYSTEM_NEED_FSTREAM.
I made some suggestions on the PR, let me know what you think about them. I can live with either way but I think the larger refactoring I suggest leaves us with something a bit clearer.
| // sendfile and copy_file_range need to fall back | ||
| // to the fstream implementation for special files | ||
| #if (defined(_LIBCPP_FILESYSTEM_USE_SENDFILE) || defined(_LIBCPP_FILESYSTEM_USE_COPY_FILE_RANGE) || \ | ||
| defined(_LIBCPP_FILESYSTEM_USE_FSTREAM)) && \ | ||
| _LIBCPP_HAS_LOCALIZATION | ||
| # include <fstream> | ||
| # define _LIBCPP_FILESYSTEM_NEED_FSTREAM | ||
| #endif |
There was a problem hiding this comment.
| // sendfile and copy_file_range need to fall back | |
| // to the fstream implementation for special files | |
| #if _LIBCPP_HAS_LOCALIZATION | |
| # include <fstream> | |
| # define _LIBCPP_FILESYSTEM_HAS_FSTREAM_FALLBACK | |
| #endif |
| namespace detail { | ||
| namespace { | ||
|
|
||
| #if defined(_LIBCPP_FILESYSTEM_NEED_FSTREAM) |
There was a problem hiding this comment.
| #if defined(_LIBCPP_FILESYSTEM_HAS_FSTREAM_FALLBACK) |
| ec.clear(); | ||
| # endif | ||
|
|
||
| # if defined(_LIBCPP_FILESYSTEM_NEED_FSTREAM) |
There was a problem hiding this comment.
| # if defined(_LIBCPP_FILESYSTEM_HAS_FSTREAM_FALLBACK) |
| return true; | ||
| } | ||
| #elif defined(_LIBCPP_FILESYSTEM_USE_FSTREAM) | ||
| #elif defined(_LIBCPP_FILESYSTEM_USE_FSTREAM) && defined(_LIBCPP_FILESYSTEM_NEED_FSTREAM) |
There was a problem hiding this comment.
| #elif defined(_LIBCPP_FILESYSTEM_USE_FSTREAM) && defined(_LIBCPP_FILESYSTEM_NEED_FSTREAM) | |
| #elif defined(_LIBCPP_FILESYSTEM_USE_FSTREAM) |
| #elif defined(_LIBCPP_FILESYSTEM_USE_FSTREAM) | ||
| #elif defined(_LIBCPP_FILESYSTEM_USE_FSTREAM) && defined(_LIBCPP_FILESYSTEM_NEED_FSTREAM) | ||
| bool copy_file_impl(FileDescriptor& read_fd, FileDescriptor& write_fd, error_code& ec) { | ||
| return copy_file_impl_fstream(read_fd, write_fd, ec); |
There was a problem hiding this comment.
| return copy_file_impl_fstream(read_fd, write_fd, ec); | |
| # if !defined(_LIBCPP_FILESYSTEM_HAS_FSTREAM_FALLBACK) | |
| # error copy_file intends to use std::fstream as a fallback, but std::fstream is not available in this configuration | |
| # endif | |
| return copy_file_impl_fstream(read_fd, write_fd, ec); |
copy_file_impl_fstreamis declared and defined only under_LIBCPP_FILESYSTEM_NEED_FSTREAM(which additionally requires_LIBCPP_HAS_LOCALIZATION), and then used in acopy_file_implvariant under a disjoint set of conditions. This causes any build that disables localization and elects this variant ofcopy_file_impl, as is currently the case when targeting wasi, to fail with a generic compiler error:Instead, since this particular
copy_file_implimplementation merely forwards tocopy_file_impl_fstream, tweak its gating condition such that the aforementioned configuration hits the dedicated fallback branch, which offers a somewhat clearer error message: