[libcxx-commits] [PATCH] D120727: [libc++] Overhaul how we select the ABI library

[Petr Hosek via Phabricator via libcxx-commits]

phosek added inline comments.


================
Comment at: libcxx/CMakeLists.txt:250
+
+set(LIBCXX_SUPPORTED_ABI_LIBRARIES none libcxxabi system-libcxxabi libcxxrt libstdc++ libsupc++ vcruntime)
+set(LIBCXX_CXX_ABI "${LIBCXX_DEFAULT_ABI_LIBRARY}" CACHE STRING "Specify C++ ABI library to use. Supported values are ${LIBCXX_SUPPORTED_ABI_LIBRARIES}.")
----------------
All of these values refer to system ABI libraries except for libcxxabi where `libcxxabi is the in-tree one and `system-libcxxabi` is the system one. From a consistency perspective, I think it'd be better for `libcxxabi` to also refer to a system ABI library, and then use a different name for the in-tree one, perhaps `default`?


================
Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:76-77
+
+  add_library(libcxx-abi-shared SHARED IMPORTED GLOBAL)
+  target_link_libraries(libcxx-abi-shared INTERFACE libcxx-abi-headers)
+  import_shared_library(libcxx-abi-shared "${LIBCXX_CXX_ABI_LIBRARY_PATH}" stdc++)
----------------
I'd consider moving these two lines into `import_shared_library` to further reduce duplication.


================
Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:80-81
+
+  add_library(libcxx-abi-static STATIC IMPORTED GLOBAL)
+  target_link_libraries(libcxx-abi-static INTERFACE libcxx-abi-headers)
+  import_shared_library(libcxx-abi-static "${LIBCXX_CXX_ABI_LIBRARY_PATH}" stdc++)
----------------
The same here, I'd consider moving these two lines into `import_static_library`.


================
Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:82
+  target_link_libraries(libcxx-abi-static INTERFACE libcxx-abi-headers)
+  import_shared_library(libcxx-abi-static "${LIBCXX_CXX_ABI_LIBRARY_PATH}" stdc++)
+
----------------
I think this should be `static`?


================
Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:86
+elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libsupc++")
+add_library(libcxx-abi-headers INTERFACE)
+  import_private_headers(libcxx-abi-headers "${LIBCXX_CXX_ABI_INCLUDE_PATHS}"
----------------
The indentation here is off.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120727