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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
