EricWF added inline comments.
================
Comment at: CMakeLists.txt:388
+# non-debug DLLs
+remove_flags("/D_DEBUG" "/MTd" "/MDd" "/MT" "/Md" "/RTC1")
+
----------------
smeenai wrote:
> cl (and presumably clang-cl) also accepts all these flags with a leading dash
> instead of a leading slash, and cmake at least has a tendency to do `-D`
> instead of `/D`, so you might need to take those into account as well. Also,
> what about the other potential `/RTC` options?
My goal here is only to remove flags which CMake adds by default as part of
`CMAKE_CXX_FLAGS_<BUILD_TYPE>_INIT`.
================
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:
> 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?
================
Comment at: lib/CMakeLists.txt:111
+ add_library_flags(ucrt) # Universal C runtime
+ add_library_flags(vcruntime) # C++ runtime
+ add_library_flags(msvcrt) # C runtime startup files
----------------
smeenai wrote:
> halyavin wrote:
> > smeenai wrote:
> > > Idk if there's anything specific to C++ in vcruntime; it's more compiler
> > > runtime functions as far as I know.
> > It contains exception handling stuff.
> You're right, but it also contains `longjmp`, `memcpy`, `memmove`, `memset`,
> etc, which is why I found the comment slightly weird initially. I guess it's
> fairly accurate as far as the usage of vcruntime in libc++ goes though.
FYI here is the documentation I was reading when deciding what libraries to
link: https://msdn.microsoft.com/en-us/library/abx4dbyh.aspx
================
Comment at: lib/CMakeLists.txt:113
+ add_library_flags(msvcrt) # C runtime startup files
+ add_library_flags(iso_stdio_wide_specifiers)
+endif()
----------------
smeenai wrote:
> Maybe add a comment explaining the purpose of this one as well?
Not sure what the comment should read. I originally implemented this patch
without this library, but during testing it generated a undefined reference to
`___PLEASE_LINK_WITH_iso_stdio_wide_specifiers.lib`. So should the comment
read "MSVC told me to link this"?
https://reviews.llvm.org/D28441
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
