hamzasood added inline comments.
================
Comment at: CMakeLists.txt:550
+endif()
add_compile_flags_if_supported(
+ -Wextra -W -Wwrite-strings
----------------
EricWF wrote:
> Couldn't we keep the "-Wall" here? And just add something else that adds
> "/W4"?
>
> Also, we don't yet support building with MSVC's frontend. Only clang-cl. Does
> clang-cl not like this existing code?
The `-Wall` flag gets converted to `-Weverything` by Clang-CL; I don’t think we
should keep it (it causes thousands of warnings per header).
`/W4` is the equivalent flag on Windows.
================
Comment at: include/filesystem:1396
- _LIBCPP_FUNC_VIS
void __create_what(int __num_paths);
----------------
EricWF wrote:
> hamzasood wrote:
> > 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.
> Perhaps part of the problem here is that filesystem builds to a static
> library, not a shared one.
>
> None the less, this macro is important once we move the definitions from
> libc++fs.a to libc++.so.
I don’t think member functions are meant to have function visibility, and I
wasn’t able to find any other such cases in libc++. That’ll always be a hard
error on Windows because a dllimport class can’t have dllimport member
functions.
================
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
----------------
EricWF wrote:
> hamzasood wrote:
> > 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.
>
> What tests are failing?
Any test that requires `TEST_HAS_C11_FEATURES` or `TEST_HAS_TIMESPEC_GET`.
Those test macros aren’t in sync with the corresponding `__config` ones.
================
Comment at: utils/libcxx/test/config.py:518
self.cxx.compile_flags += ['-DNOMINMAX']
+ # Disable auto-linking; the library is linked manually when
+ # configuring the linker flags.
----------------
EricWF wrote:
> I think the correct fix here is to disabling linking libc++ when auto linking
> is enabled by the headers. Because we want to test that the auto link pragmas
> work.
In that case, it might be worth adding separate auto-linking tests that
specifically use Clang-CL. Making it work here would involve defining `_DLL`
for dynamic builds, but I’m not sure if that would interfere with the CRT.
https://reviews.llvm.org/D51868
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
