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

[Artem Belevich via llvm-commits]

tra added a comment.

In http://reviews.llvm.org/D20221#433597, @MatzeB wrote:

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


This argument applies to passing these values via global variables -- both llvm_singlesource() and llvm_multisource() use specific variables to pass their values to specific cmake commands. Whole parsing code is pretty much mechanical conversion from global variable name to its local __ARG_xxx variant. While it is possible to get them out of sync, it's not going to be big deal to find a fix it if that happens.

> Our usage in the llvm_multisource() case is now different from the usage of the llvm_singlesource() case now.


Not really. Parsing that matters only happens in test_suite_add_executable(). llvm_* just cherry-pick two args and pass through the rest. Those two args are only needed to deal with idiosyncrasies of current interface (current files use PROG and MAIN arguments in rather peculiar way). That can be fixed and then there will be no difference between llvm_singlesource and multisource parsing-wise.

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


It may be an option, though I'm not convinced we need it.

> 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,


DEPS and LIBS I've added to be used in CUDA tests (http://reviews.llvm.org/D19434) where I need both extra build dependencies and libraries.
Using cmake API was rather cumbersome. Details are below.

> 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()`


Ah, but ${prog} is *not* the target we create.

PROG is currently just something we use to check against blacklist of entities to skip.
Real target is result of get_unique_exe_name(executable ${_ARG_MAIN}) and we just rely on the fact that MAIN is either set to be ${PROG}.c which usually happens to be converted back to equivalent of ${PROG} by get_unique_exe_name() or, in case of llvm_single_source() we derive PROG from the name of the source file. In trivial cases ${PROG} == ${MAIN}.c, but that's not true if, for instance, we need to add unique prefix. So, your example above will work for *some* test cases, but not for all.

The problem is that it all happens under the hood and we, generally speaking, don't know the name of real target created by llvm_multisource() or llvm_singlesource() and thus can't use regular cmake API to augment rules to build that target.

Perhaps that's the first thing I should fix. Once we have a way to figure out the name of the target we've generated, we can do custom things outside of SingleMultiSource.cmake. It will also make it possible to make llvm_singlesource to be just a wrapper over llvm_multisource with a single source file and thus avoid divergence you're concerned about.


http://reviews.llvm.org/D20221