[llvm-commits] [Review request] CMake: Add the new option "LLVM_LIT_ARGS"

[Óscar Fuentes]

Hello Takumi.

NAKAMURA Takumi <geek4civic at gmail.com> writes:

> I propose adding, for CMake, the option "LLVM_LIT_ARGS".
>
> By default LLVM_LIT_ARGS is set;
>
> MSVC or XCODE: -sv --no-progress-bar
> others: -sv
>
> *  0001-CMake-Add-the-new-option-LLVM_LIT_ARGS.patch
>
>   for llvm tree.
>
> *  0001-test-CMakeLists.txt-Use-LLVM_LIT_ARGS-and-remove-red.patch
>
>   for clang tree. it depends on the former patch.

Unless I'm missing something on how cmake processes options, there is an
use-before-first-assignment problem here.

LLVM_LIT_ARGS is defined in test/CMakeLists.txt, but it is used in
tools/clang/test/CMakeLists.txt, which is processed before
test/CMakeLists.txt. If the user sets LLVM_LIT_ARGS from the cmake
command line there is no problem, but if he doesn't, the default value
you set in test/CMakeLists.txt is not used for clang.

You can test this by starting from a new build directory without using
LLVM_LIT_ARGS on the command line, and checkig if the -sv argument is
passed to lit on the clang tests.

> I wonder whether the name "LLVM_LIT_ARGS" would be suitable or not :)
> (would be better to split to new namespace LIT_**** ?)
> Please gimme any advices.

The variable is defined on a LLVM/Clang CMakeLists.txt file, so it
belongs to the LLVM namespace. The LIT namespace would make sense iif
LIT had its own CMakeLists.txt files. IMO.

I'm wary by the use of separate_arguments. Why do you need it and why
UNIX_COMMAND was chosen?

On the clang part, the variable CLANG_TEST_EXTRA_ARGS is referenced by

set(CLANG_TEST_EXTRA_ARGS)

(which is equivalent to unsetting it) and then you

+  list(APPEND CLANG_TEST_EXTRA_ARGS ${LIT_ARGS})

It would be clearer if the set(CLANG_TEST_EXTRA_ARGS) were replaced by

+  set(CLANG_TEST_EXTRA_ARGS ${LIT_ARGS})

or replacing the occurrences of CLANG_TEST_EXTRA_ARGS by LIT_ARGS
altogether, as it no longer holds the "extra args" but all the
configurable lit args for clang.

Also, it would be a good thing to add documentation for the
LLVM_LIT_ARGS variable in docs/CMake.html.