[libcxx-commits] [PATCH] D89530: [libcxx] [test] Fix filesystem_test_helper.h to compile for windows

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/test/support/filesystem_test_helper.h:26
 // For creating socket files
-#if !defined(__FreeBSD__) && !defined(__APPLE__)
+#if !defined(__FreeBSD__) && !defined(__APPLE__) && !defined(_WIN32)
 # include <sys/socket.h>
----------------
ldionne wrote:
> I'm tempted to move all of those to
> 
> ```
> #if __has_include(<foo>)
> #  include <foo>
> #endif
> ```
> 
> With appropriate comments about what we're getting from those headers, it seems like this is easier to follow than complicated platform-specific `#if`s.
> 
There are matching sets of ifdefs further down in this file, around the `create_socket` function, and within the tests in fs.op.funcs/fs.op.status/status.pass.cpp and fs.op.funcs/fs.op.symlink_status/symlink_status.pass.cpp as well. But maybe with `__has_include()` here, and a `#define HAS_SOCKET` which is used elsewhere?

Also, I'm running the tests against MS STL with clang-cl, but is the test suite supposed to work with MSVC itself as well? I presume that doesn't support `__has_include()`.


================
Comment at: libcxx/test/support/filesystem_test_helper.h:31
 
+#ifdef _WIN32
+// MSVC headers warn if using the versions without a leading underscore.
----------------
ldionne wrote:
> Instead, I would create a layer of indirection for the utilities we're using here like this:
> 
> ```
> namespace utils {
> 
> #if defined(_WIN32)
>   inline std::string getcwd() { /* windows impl */ }
>   inline int mkdir(char const*, int) { /* windows impl */ }
>   // etc
> #else
>   inline std::string getcwd() { /* posix impl */ }
>   inline int mkdir(char const*, int) { /* posix impl */ }
>   // etc
> #endif
> 
>   inline bool exists(std::string const& path) { /* common impl */ }
> } // end namespace utils
> ```
> 
> It's more verbose, but easier to follow and the code below can make unconditional calls to these functions without being cluttered by `#if`s.
Hmm, I can agree with the clarity, but for these functions, there's no `#if`s anywhere in the calling code. Especially the duplication for getcwd feels a bit harsh, when the difference is just between `::getcwd` and `::_getcwd`.

I thought we could get rid of a few of these (that just add the underscore prefix) by defining `_CRT_NONSTDC_NO_WARNINGS` though, leaving just mkdir and ftruncate. But that ends up requiring manually linking against `oldnames.lib` (which normally is linked implicitly, but not in the setup things are done in the libc++ test suite)...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89530