[PATCH] D51868: [libcxx] Build and test fixes for Windows

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added inline comments.


================
Comment at: include/filesystem:1396
 
-  _LIBCPP_FUNC_VIS
   void __create_what(int __num_paths);
----------------
compnerd wrote:
> This possibly changes the meaning on other targets.  What was the error that this triggered?
I've re-uploaded the patch with full context to make this clearer.

That's a member function on an exported type, which I don't think should have any visibility attributes. The specific issue in this case is that both the class type and the member function are marked as dllimport, which causes a compilation error.


================
Comment at: test/support/test_macros.h:147
-#  elif defined(_WIN32)
-#    if defined(_MSC_VER) && !defined(__MINGW32__)
-#      define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library
----------------
STL_MSFT wrote:
> compnerd wrote:
> > I think that the condition here is inverted, and should be enabled for MinGW32.
> What error prompted this? It hasn't caused problems when running libc++'s tests against MSVC's STL (with both C1XX and Clang).
The comment above this says that it's copied from `__config`, but they must've gone out of sync at some point because `__config` doesn't have that extra section that I deleted.

This causes lots of errors when testing libc++. E.g. libc++ isn't declaring `std::timespec` on Windows because `_LIBCPP_HAS_C11_FEATURES` isn't defined, but the tests think that it's available so they try to use it (which causes compilation failures in lots of tests).

The better solution here might be to update `__config` to match this, but I'm not familiar with some of those platforms so this seemed like the safest approach for now.


================
Comment at: test/support/test_macros.h:147
-#  elif defined(_WIN32)
-#    if defined(_MSC_VER) && !defined(__MINGW32__)
-#      define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library
----------------
hamzasood wrote:
> STL_MSFT wrote:
> > compnerd wrote:
> > > I think that the condition here is inverted, and should be enabled for MinGW32.
> > What error prompted this? It hasn't caused problems when running libc++'s tests against MSVC's STL (with both C1XX and Clang).
> The comment above this says that it's copied from `__config`, but they must've gone out of sync at some point because `__config` doesn't have that extra section that I deleted.
> 
> This causes lots of errors when testing libc++. E.g. libc++ isn't declaring `std::timespec` on Windows because `_LIBCPP_HAS_C11_FEATURES` isn't defined, but the tests think that it's available so they try to use it (which causes compilation failures in lots of tests).
> 
> The better solution here might be to update `__config` to match this, but I'm not familiar with some of those platforms so this seemed like the safest approach for now.
This is a workaround for a libc++ issue rather than an MSVC one. See the response to @compnerd for the full details.


================
Comment at: test/support/verbose_assert.h:24
         : (IsStreamable<decltype(std::wcerr), Tp>::value ? 2 : -1))>
-struct SelectStream {
+struct SelectErrStream {
   static_assert(ST == -1, "specialization required for ST != -1");
----------------
mclow.lists wrote:
> Why the renaming here? 
> 
> What's the deal with sometimes using `clog` and sometimes `cerr`?
> (I know, you didn't do this, but ???)
> 
Some of the casting that goes on in that template triggers an error under `-fno-ms-extensions` when given a function pointer (in this case: `std::endl`). Selecting between a standard/wide stream isn't needed for `std::endl` as it can go to either, so I've bypassed the selection by sending it straight to `cerr`.

The renaming is to clarify that an `stderr` stream (and nothing else) is being selected, which sort of justifies going directly to `cerr`.


https://reviews.llvm.org/D51868