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

[Matthias Braun via llvm-commits]

MatzeB added a comment.

In http://reviews.llvm.org/D20221#433658, @tra wrote:

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


I completely agree with you that the current global variable approach should be changed. It is hard to specify flags on a per-target basic, it is also hard to document/learn what variables are available and what their effect is. There is also no way to catch typos in variable names.

To give some context: I imagine that we will see macros like llvm_cuda(), llvm_halide(), llvm_googletest() popping up in the future. Those would basically call llvm_multisource() and perform some extra actions or add some default flags.

I can see pros and cons with either passing flags with additional function calls mentioning the target name or by adding additional named arguments to a single macro call:

- Functions are easy to discover and document. Named arguments may be interpreted by various functions which is harder to track and understand.
- Typos are easily diagnosed with function ("unknown function") with named arguments they will be parsed into the wrong list which may lead to hard to understand follow-up errors.
- The named argument name may clash with a compiler flag we want to use. This is probably so unlikely we can ignore it, but if it would ever happen it would be hard to work around.
- Named arguments give you a single list with all flags and settings which is easy to duplicate. This is useful to programmatically create multiple variants of a test.
- Named arguments force us to collect all flags/variables first and pass them along in a single call.
  - This can be nice because we have a single point to check for errors and consistency and are guaranteed to have no ordering problems.
  - It can also be annoying because it mostly enforces an order on you (no adding of a few flags afterwards).
- You need to type less with named arguments. The functions approach requires you to repeatedly mention the target name with each function call.
- The core cmake macros look more like the function approach.
- I can imagine work arounds for any of the problems mentioned above in both approaches (some simple and obvious some leading to ugly code).

I don't see any strong argument either way but have a weak tendency towards the function approach.

> 

> 

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


I completely agree here, I would describe it as: llvm_multisource() should not exist and people should just directly call test_suite_add_executable() instead. (llvm_singlesource() is already a wrapper over that).

We really should have a way to know the target name. Maybe have a function that returns the target name when given a test name or maybe we can restrict the usage of llvm_target_prefix() to llvm_singlesource() so that at least you have a natural correspondence between test name and target name for all other cases...


http://reviews.llvm.org/D20221