beanz added a comment.
I will send updated patches shortly.
Comments inline below.
================
Comment at: runtime/CMakeLists.txt:33
@@ -32,1 +32,3 @@
+ )
+ set(cmake_3_4_USES_TERMINAL USES_TERMINAL 1)
endif()
----------------
samsonov wrote:
> Should this also be named cmake_3_4_USES_TERMINAL_OPTIONS?
This was used in an earlier iteration to pass USES_TERMINAL into a call to
`ExternalProject_Add_Step` I will remove it.
================
Comment at: runtime/CMakeLists.txt:40
@@ +39,3 @@
+ set(STAMP_DIR ${CMAKE_CURRENT_BINARY_DIR}/compiler-rt-stamps/)
+ set(BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/compiler-rt-bins/)
+
----------------
samsonov wrote:
> This one is later re-defined from ExternalProject_Get_Property. Does it
> provide the same value there?
Yep, it can be removed.
================
Comment at: runtime/CMakeLists.txt:42
@@ +41,3 @@
+
+ add_custom_target(compiler-rt-clear
+ DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/compiler-rt-cleared
----------------
samsonov wrote:
> Where do you use this target?
That is a convenience target for clearing out the compiler-rt directory. It
isn't strictly needed, but I have found it (and the bootstrap-clear) to be
useful.
Particularly as I've been changing which files are generated by the compiler-rt
build and where they end up it is nice to be able to nuke the build directory.
================
Comment at: runtime/CMakeLists.txt:47
@@ +46,3 @@
+ OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/compiler-rt-cleared
+ DEPENDS clang llvm-config
+ COMMAND ${CMAKE_COMMAND} -E remove_directory ${BINARY_DIR}
----------------
samsonov wrote:
> Why do you need these deps?
Specifying those deps makes it so that if llvm-config or clang change
compiler-rt gets cleaned out and rebuilt.
================
Comment at: runtime/CMakeLists.txt:71
@@ -49,2 +70,3 @@
+ -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}
INSTALL_COMMAND ""
STEP_TARGETS configure build
----------------
samsonov wrote:
> Why did you remove -DCOMPILER_RT_ENABLE_WERROR=ON? I think it's fine to keep
> it enabled, as it's ok to ensure that ToT Clang builds ToT compiler-rt with
> no warnings.
I'm trying to set the minimal set of options required to make compiler-rt
build. Any additional options can be passed in via
`CLANG_COMPILER_RT_CMAKE_ARGS`.
I think that if we expect `COMPILER_RT_ENABLE_WERROR` to be the way everyone is
building, we should make that the default in compiler-rt, not pass it in here.
http://reviews.llvm.org/D13399
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
