[PATCH] D61356: [compiler-rt] Rework the object build support

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: compiler-rt/cmake/Modules/AddCompilerRT.cmake:278
-      string(TOUPPER ${CMAKE_BUILD_TYPE} config)
-      get_property(cflags SOURCE ${sources_${libname}} PROPERTY COMPILE_FLAGS)
-      separate_arguments(cflags)
----------------
smeenai wrote:
> smeenai wrote:
> > I think you mean "CMake 3.9" and not "LLVM 3.9" :)
> Is it okay to drop this part? I don't know how common it is to set `COMPILE_FLAGS` on sources.
I don't think it's very often, but I'm fine including it.


================
Comment at: compiler-rt/cmake/Modules/AddCompilerRT.cmake:278
+      # TODO: CMake 3.4 doesn't allow installing library objects so we have to build them manually.
+      # When LLVM migrates to LLVM 3.9, we can use add_library just like for other library types.
+      if(CMAKE_C_COMPILER_ID MATCHES Clang AND CMAKE_C_COMPILER_TARGET)
----------------
phosek wrote:
> smeenai wrote:
> > smeenai wrote:
> > > I think you mean "CMake 3.9" and not "LLVM 3.9" :)
> > Is it okay to drop this part? I don't know how common it is to set `COMPILE_FLAGS` on sources.
> I don't think it's very often, but I'm fine including it.
Yes, but after looking into it a bit I realized that this may not be enough since CMake doesn't allow specifying output names for object libraries so I deleted the comment.


================
Comment at: compiler-rt/cmake/Modules/AddCompilerRT.cmake:304
+      endforeach()
+      string(REPLACE " " ";" compile_command_${libname} "${compile_command_${libname}}")
       add_custom_command(
----------------
smeenai wrote:
> Would `separate_arguments` be better here, to handle quoted arguments etc. correctly? (Though I guess the `string(REPLACE)` on line 282 is potentially screwing up quoted arguments anyway.)
It seems to be working.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D61356