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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
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>
----------------
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.



================
Comment at: libcxx/test/support/filesystem_test_helper.h:31
 
+#ifdef _WIN32
+// MSVC headers warn if using the versions without a leading underscore.
----------------
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.


================
Comment at: libcxx/test/support/filesystem_test_helper.h:253
     static_test_env() {
+#ifndef _WIN32
         env_.create_symlink("dne", "bad_symlink", false);
----------------
What are we missing to handle those correctly? I would rather not `#ifdef` them out if it's just "until we've fixed it". Or, at least, please put up another patch that removes them and mark it as a child of this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89530