[PATCH] D13739: [libcxx] Make libc++.so a linker script by default on most platforms.

[Jonathan Roelofs via cfe-commits]

jroelofs added inline comments.

================
Comment at: CMakeLists.txt:71
@@ -70,1 +70,3 @@
+# Use a static copy of the ABI library when linking libc++. This option
+# cannot be used with LIBCXX_ENABLE_ABI_LINKER_SCRIPT.
 option(LIBCXX_ENABLE_STATIC_ABI_LIBRARY "Statically link the ABI library" OFF)
----------------
Should probably make this a cmake error, not just have it do something other than what you wanted.

================
Comment at: CMakeLists.txt:76
@@ +75,3 @@
+# will link libc++ to the correct ABI library. This option is ON by default
+# On UNIX platforms other than Apple unless 'LIBCXX_ENABLE_STATIC_ABI_LIBRARY'
+# is ON.
----------------
s/On/on/ ?

================
Comment at: CMakeLists.txt:78
@@ +77,3 @@
+# is ON.
+set(DEFAULT_VALUE OFF)
+if (LLVM_HAVE_LINK_VERSION_SCRIPT AND NOT LIBCXX_ENABLE_STATIC_ABI_LIBRARY)
----------------
`DEFAULT_VALUE` seems too generic a name for this... or is this a common CMake pattern that I'm unaware of?

================
Comment at: cmake/Modules/HandleOutOfTreeLLVM.cmake:116
@@ +115,3 @@
+    if(CYGWIN)
+      set(LLVM_ON_WIN32 0)
+      set(LLVM_ON_UNIX 1)
----------------
Where are `LLVM_ON_WIN32` and `LLVM_ON_UNIX` used?

================
Comment at: docs/BuildingLibcxx.rst:176
@@ +175,3 @@
+  **Default**: ``OFF`` On Windows and Apple or when
+  ``LIBCXX_ENABLE_STATIC_ABI_LIBRARY`` is specified and ``ON`` otherwise.
+
----------------
I find this sentence hard to parse. Maybe phrase it the same way you did in the comment: 

"ON by default on UNIX platforms other than Apple unless 'LIBCXX_ENABLE_STATIC_ABI_LIBRARY' is ON"

================
Comment at: docs/UsingLibcxx.rst:59
@@ +58,3 @@
+some libc++ installations require the user manually link libc++abi themselves.
+If you are running into linker errors when using libc++ try adding "-lc++abi"
+to the link line.  For example:
----------------
Looks like an inconsistent use of `"` vs `'` for `'-stdlib=libc++'` and `"-lc++abi"`. I don't have good RST-fu, but that seems fishy.


http://reviews.llvm.org/D13739