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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
