[libcxx-commits] [PATCH] D94824: [MSVC] Disable <fstream> usage of <filesystem>

Fri Jan 15 23:03:52 PST 2021

mstorsjo added inline comments.

================
Comment at: libcxx/include/fstream:190
+
+#if _LIBCPP_STD_VER >= 17 && !defined(_MSC_VER)
 #include <filesystem>
----------------
rnk wrote:
> mstorsjo wrote:
> > I'm not entirely fond of hardcoding the condition like this - would it be better to emit something to `__config` (via `__config_site`) about whether filesystem is disabled?
> > 
> > I guess hardcoding msvc could be argued for, because the dllexport behaviour is kinda msvc specific (this problem doesn't manifest in mingw builds, that build successfully in dll mode).
> > 
> > Another reason for not hardcoding it for msvc, is that while filesystem is disabled for now, it hopefully won't be for long, and it'd be nice not to have an extra condition to keep track of to update in sync.
> I think if the project wants to support the LIBCXX_ENABLE_FILESYSTEM option over the long run, then it probably makes sense to feed that into __config_site and use that to control this include and the availability of these overloads.
> 
> However, if we view this as a temporary portability work around, then I think it makes sense to limit the scope of the workaround as much as possible.
Fair enough. Wrt enabling filesystem in msvc configurations, the int128 builtin issue (see D91139) remains. (For mingw configurations, the patchset in review is everything that is needed.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94824