[PATCH] D20842: [test-suite] Simplified test executable name generation.

[Artem Belevich via llvm-commits]

tra added inline comments.

================
Comment at: cmake/modules/SingleMultiSource.cmake:27-31
@@ -12,1 +26,7 @@
+#
+# Results:
+#   LLVM_TEST_EXECUTABLE - name of the executable target created by the last
+#                          invocation of llvm_test_executable().
+#   LLVM_SINGLESOURCE_EXECUTABLES - names of executables created by the
+#                          last invocation of llvm_singlesource().
 ##===----------------------------------------------------------------------===##
----------------
MatzeB wrote:
> Passing function results through global variable is ugly.
> The cmake way to handle function results would be to have an additional (possibly optional) parameter that names the variable where we write the result to. That way you can at least see the connection at the call site.
> 
OK. 

================
Comment at: cmake/modules/SingleMultiSource.cmake:138
@@ -120,1 +137,3 @@
+macro(llvm_test_executable name)
+  set(LLVM_TEST_EXECUTABLE)
   list(FIND PROGRAMS_TO_SKIP ${name} name_idx)
----------------
MatzeB wrote:
> This would have needed a `PARENT_SCOPE` flag to actually work. Though I still recommend against using well known variable names to pass the result along.
llvm_*source() are macros, so is llvm_test_excutable therefore PARENT_SCOPE is not needed here.



================
Comment at: cmake/modules/SingleMultiSource.cmake:147-148
@@ -127,2 +146,4 @@
     append_compile_flags(${executable} ${CXXFLAGS})
+    target_include_directories(${executable} PUBLIC ${CMAKE_CURRENT_SOURCE_DIR})
+    target_include_directories(${executable} PUBLIC ${CMAKE_CURRENT_BINARY_DIR})
     # Note that we cannot use target_link_libraries() here because that one
----------------
MatzeB wrote:
> Why did you move this into llvm_test_executable()? It wasn't present for singlesource tests before. It also would have been a nice test to use the returned target name inside llvm_multisource() and apply `target_include_directories()` on that.
I was trying to make default compilation flags identical for both llvm_*source() variants.
I can easily move it back to llvm_multisource, if you prefer.


http://reviews.llvm.org/D20842