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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D59248#1426883 <https://reviews.llvm.org/D59248#1426883>, @smeenai wrote:

> The downside, of course, is that we end up compiling the same files twice for the static and shared libraries, but it makes sense if they're gonna have different flags.


Yes, they do have different flags. But you're right, we're doing a bit more work now. Fortunately, building libc++ is very fast.



================
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)
----------------
smeenai wrote:
> I think this can trivially be a function instead of a macro, which avoids potential future variable pollution.
Good point.


================
Comment at: libcxx/lib/CMakeLists.txt:209
     PROPERTIES
+      COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
       LINK_FLAGS    "${LIBCXX_LINK_FLAGS}"
----------------
smeenai wrote:
> 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.
I didn't know that! It's good to know.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D59248