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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/test/support/filesystem_test_helper.h:59
+#else
+    inline int mkdir(const char* path, int mode) { return ::mkdir(path, mode); }
+    inline int ftruncate(int fd, off_t length) { return ::ftruncate(fd, length); }
----------------
You could even use `using ::mkdir` here!


================
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>
----------------
mstorsjo wrote:
> 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()`.
Any support for MSVC is accidental at this point: I believe there was some amount of work done to make it work a few years back, however that is not tested on a regular basis and nobody is actively maintaining it.


================
Comment at: libcxx/test/support/filesystem_test_helper.h:31
 
+#ifdef _WIN32
+// MSVC headers warn if using the versions without a leading underscore.
----------------
mstorsjo wrote:
> 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)...
I think we should link against it manually if that's what's done automatically normally. That makes us closer to running the compiler the way it's run by default.


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

https://reviews.llvm.org/D89530