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

[Matthias Braun via llvm-commits]

MatzeB 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().
 ##===----------------------------------------------------------------------===##
----------------
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.


================
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)
----------------
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.

================
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
----------------
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.

================
Comment at: cmake/modules/SingleMultiSource.cmake:178
@@ -155,2 +177,3 @@
 macro(llvm_singlesource)
+  set(LLVM_SINGLESOURCE_EXECUTABLES)
   file(GLOB sources *.c *.cpp *.cc)
----------------
see above.


http://reviews.llvm.org/D20842