[PATCH] D20221: [test-suite] parameterization of llvm_{multi, single}source()

[Matthias Braun via llvm-commits]

MatzeB added a comment.

I like this part of the patch (in fact I wanted to a similar thing myself at some point): I think it would be nice to llvm_multisource() to mostly behave like `add_executable()` so it would force you to mention the PROG name and allow you to specify sourcefiles behind that without an explicit `PROG` or `SOURCES` prefix. I can understand though that you may want to first transition to this to fixup the CMakeLists.txt files and only later remove the prefixes...

Adding `CFLAGS`, `CPPFLAGS`, `LDFLAGS`, ... as optional arguments does indeed reduce the boilerplate, however I am concerned that this is the wrong interface. If this becomes common usage we will need to add each new variable we add in the future to llvm_multisource() or we will have a strange disconnect between variable and named argument usage again. If someone adds new variants of llvm_multisource() for some reason he has to replicate all the parsing logic and pass those values through. Our usage in the llvm_multisource() case is now different from the usage of the llvm_singlesource() case now. Maybe we should rather aim for something like:
`llvm_multisource(foo)
llvm_add_cflags(foo ...)
llvm_add_cxxflags(foo ...)` and once we have that provide some convencience version that combines typical options:
`llvm_add_flags(foo CFLAGS ... CXXFLAGS ...)`

Simply using `target_link_libraries(foo barlib)` should be nearly as simple as adding a `LIBS` argument but has the added benefit that it is the documented way to do it in cmake and is obvious to people familiar with cmake. Similarily with `DEPS`. It seems like those two flags are not used in most cases, so I think it would be better if you provide a convenience macro around those for the cases that do need them. Something along the lines of:
`macro(llvm_multisource_dep_libs)

  ... parse DEPS, LIBS ...
  llvm_multisource(${prog} ...)
  target_link_libraries(${prog} ...)
  add_dependencies(${prog} ...)

endmacro()`


http://reviews.llvm.org/D20221