rnk added inline comments.
================ Comment at: CMakeLists.txt:43 +if (WIN32 AND NOT MINGW) + set(LIBCXX_TARGETING_WINDOWS ON) +else() ---------------- smeenai wrote: > EricWF wrote: > > EricWF wrote: > > > smeenai wrote: > > > > Not the biggest fan of this name, since it's not obvious why MinGW > > > > shouldn't count as targeting Windows. I thought of > > > > `LIBCXX_TARGETING_NATIVE_WINDOWS` or `LIBCXX_TARGETING_MSVCRT` instead, > > > > but MinGW is also native Windows and targets MSVCRT, so those names > > > > aren't any better from that perspective either. I can't think of > > > > anything else at the moment, but I'm sure there's a better name. > > > Thanks for the feedback. I'm not exactly sure how this macro should be > > > defined either. > > > > > > I thought that `MinGW` provided its own C library runtime wrappers that > > > forward to `MSVCRT`. > > > > > > The difference I was imagining between Native Windows builds and `MinGW` > > > is that it's possible to use > > > `pthread` with `MinGW` but not with native Windows targets. > > Another distinction I would like this macro to embody is whether on not the > > compiler defines `_MSC_VER`. `clang-cl`, `clang++` on Windows, and MSVC > > `cl` all define this macro. > If I recall correctly, MinGW uses `mscvrt.dll` but not `msvcrt.lib` (see my > other comment for the distinction). I'm fine with this name for now then; we > can always bikeshed later. > > Btw it's also possible to use pthread with native Windows targets, via > [pthreads-win32](http://www.sourceware.org/pthreads-win32/) or other > libraries. I'd call this LIBCXX_TARGETING_MSVCRT. "msvcrt.dll" usually refers to an ancient version (6?) of the Visual C runtime that is distributed as part of Windows. Typically it is found at C:/Windows/system32/msvcrt.dll. Mingw uses this, because it is available on all Windows systems they care to support. This DLL basically never receives updates, so I wouldn't want to build libc++ functionality on top of it. Over time, it seems that mingw has had to implement more and more C runtime functionality itself, and I wouldn't want libc++ to have to do that. Until recently, MSVC users were required to redistribute the version of the CRT they chose to link against, and it was expected that all DLLs sharing CRT resources had to link against the same CRT version. Of course, this caused problems, so they went back to the single, OS-provided CRT in VS 2015 (https://blogs.msdn.microsoft.com/vcblog/2015/03/03/introducing-the-universal-crt/). My conclusion is that it's no longer worth thinking of that old DLL as the Visual C runtime. It's just an artifact of history at this point. If you say libc++ targets the MSVCRT, you're probably talking about the modern, universal CRT. --- If you want this option to imply both the C++ ABI and the C runtime, then LIBCXX_TARGETING_MSVC would probably be a more appropriate name. Clang and LLVM have some support for using the Itanium C++ ABI with the MSVCRT, but it is more experimental, and not ABI compatible with any existing software. Saleem could say more about where he thinks this target will go in the future and whether it's worth adding to the libc++ support configuration matrix. ================ Comment at: lib/CMakeLists.txt:108-109 +if (LIBCXX_TARGETING_WINDOWS) + add_compile_flags(/Zl) + add_link_flags(/nodefaultlib) + add_library_flags(ucrt) # Universal C runtime ---------------- smeenai wrote: > EricWF wrote: > > smeenai wrote: > > > halyavin wrote: > > > > smeenai wrote: > > > > > These should be guarded under a check for a cl or cl-like frontend > > > > > rather than `LIBCXX_TARGETING_WINDOWS` (since in theory we could be > > > > > using the regular clang frontend to compile for Windows as well). > > > > Regular clang supports both gcc-like and cl-like options (there are 2 > > > > compilers: clang.exe and clang-cl.exe). I think it is not worth it to > > > > support both considering they differ only in command line options > > > > handling. > > > I'm aware of the separate drivers, but I still think it's worthwhile > > > specifying appropriate conditionals when it's easy enough to do. (In this > > > case, the inverse check of > > > https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/CMakeLists.txt;291339$394 > > > should do the trick.) > > Is it alright if libc++'s CMakeLists.txt only supports targeting `clang-cl` > > for the time being? I would love to also support `clang++` but that seems > > like a ways off. > > > > For now my only concern is getting `clang-cl` working. Expanding to include > > supporting `clang++` seems like it's a ways away. @smeenai Would this be a > > regression for you? > It would. It's easy enough to work around locally, so I don't care too much, > but it's also easy enough to not regress in the first place :p Would using > `add_flags_if_supported` and `add_link_flags_if_supported` instead of the > unconditional versions here work? Any reason not to use `if (MSVC)`? That generally implies that the compiler supports MSVC flags. It's true for clang-cl, at least. If you don't like that, we should probably make some attempt to identify the linker command line syntax and use the appropriate flag. https://reviews.llvm.org/D28441 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
