[libcxx-commits] [PATCH] D98367: [libcxxabi] Use cxx-headers target to consume libcxx headers

[Saleem Abdulrasool via Phabricator via libcxx-commits]

compnerd added inline comments.


================
Comment at: libcxxabi/CMakeLists.txt:163
     "Specify path to libc++ includes.")
-message(STATUS "Libc++abi will be using libc++ includes from ${LIBCXXABI_LIBCXX_INCLUDES}")
+if (LIBCXXABI_STANDALONE_BUILD)
+  if (NOT IS_DIRECTORY ${LIBCXXABI_LIBCXX_INCLUDES})
----------------
Why not make this `if (NOT TARGET cxx-headers)`?  You don't really care so much about the standalone target as much as you do about the `cxx-headers` target existing.


================
Comment at: libcxxabi/CMakeLists.txt:168
+      "Please provide the path to where the libc++ headers have been installed.")
+  endif()
+  add_library(cxx-headers INTERFACE)
----------------
I believe that with the suggestion below, this check is unnecessary as CMake will verify that the path exists and is a directory.


================
Comment at: libcxxabi/CMakeLists.txt:170-174
+  if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC" OR "${CMAKE_CXX_SIMULATE_ID}" STREQUAL "MSVC")
+    target_compile_options(cxx-headers INTERFACE /I "${LIBCXXABI_LIBCXX_INCLUDES}")
+  else()
+    target_compile_options(cxx-headers INTERFACE -I "${LIBCXXABI_LIBCXX_INCLUDES}")
+  endif()
----------------
There is no need for this complicated special handling, why not just let CMake handle it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98367