[libcxx-commits] [PATCH] D59248: [libc++] Do not share an object library to create the static/shared libraries

[Shoaib Meenai via Phabricator via libcxx-commits]

smeenai added inline comments.


================
Comment at: libcxx/lib/CMakeLists.txt:169
 
-split_list(LIBCXX_COMPILE_FLAGS)
-split_list(LIBCXX_LINK_FLAGS)
-
-macro(cxx_object_library name)
-  cmake_parse_arguments(ARGS "" "" "DEFINES;FLAGS" ${ARGN})
-
-  # Add an object library that contains the compiled source files.
-  add_library(${name} OBJECT ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS})
+macro(cxx_set_common_defines name)
   if(LIBCXX_CXX_ABI_HEADER_TARGET)
----------------
I think this can trivially be a function instead of a macro, which avoids potential future variable pollution.


================
Comment at: libcxx/lib/CMakeLists.txt:209
     PROPERTIES
+      COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
       LINK_FLAGS    "${LIBCXX_LINK_FLAGS}"
----------------
ldionne wrote:
> phosek wrote:
> > CMake documentation says that [`COMPILE_FLAGS` is deprecated](https://cmake.org/cmake/help/v3.4/prop_tgt/COMPILE_FLAGS.html) and that we should be using `COMPILE_OPTIONS` or `target_compile_options`.
> If that's okay, I'd like to do that as a follow-up change. I'd do:
> 
> ```
> target_compile_options(cxx_shared PRIVATE ${LIBCXX_COMPILE_FLAGS})
> target_link_options(cxx_shared PRIVATE ${LIBCXX_LINK_FLAGS})
> ```
> 
> The reason is that if this somehow breaks something (I'm worried that `target_compile_options` is additive where setting the property might overwrite the compile flags for that target, which could be different), at least I won't have to revert the whole patch.
One difference to be aware of (and this is **really** annoying behavior on CMake's part IMO) is that `COMPILE_OPTIONS` are deduplicated, such that if you e.g. tried setting `COMPILE_OPTIONS` to `-isystem foo -isystem bar`, CMake would transform into `-isystem foo bar`, which of course breaks horribly. Hopefully you won't run into that, but it's something to be aware of.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D59248