[libcxx-commits] [PATCH] D118503: [SystemZ][z/OS] Add ASCII and 32-bit variants for libc++.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Only a few nits.



================
Comment at: libcxx/CMakeLists.txt:480-481
 endif()
+add_compile_flags("${LIBCXX_ADDITIONAL_COMPILE_FLAGS}")
+add_library_flags("${LIBCXX_ADDITIONAL_LIBRARIES}")
 
----------------
Let's move this to the per-target functions below. Let's put it in `cxx_add_basic_build_flags`. It should then become `target_compile_options(...)` and `target_link_libraries(...)`.


================
Comment at: libcxx/cmake/caches/s390x32-ibm-zos.cmake:16
+
+set(LIBCXXABI_DLL_NAME CRTEHCXA CACHE STRING "")
+
----------------
This variable doesn't exist, why is it in the cache? If you've got internal-only changes you're keeping downstream, it looks like this should be part of that.


================
Comment at: libcxx/cmake/caches/s390x32-ibm-zos.cmake:18
+
+set(LLVM_EXTERNAL_UNWIND_SOURCE_DIR "$ENV{LLVM_BASE}/zos-unwind" CACHE PATH "The path to the external unwind source directory.")
+set(LIBCXXABI_ADDITIONAL_LIBRARIES "-Wl,lib/libunwind.x" CACHE STRING "")
----------------
Same here regarding internal-only changes.


================
Comment at: libcxxabi/CMakeLists.txt:230
 set(LIBCXXABI_LINK_FLAGS "")
 set(LIBCXXABI_LIBRARIES "")
+set(LIBCXXABI_ADDITIONAL_LIBRARIES "" CACHE STRING "Additional libraries libc++abi is linked to which can be provided in cache")
----------------
We should also have `LIBCXXABI_ADDITIONAL_COMPILE_FLAGS`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118503/new/

https://reviews.llvm.org/D118503