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

[NAKAMURA Takumi]

Good afternoon, Oscar.

I attached updated version of patches.

Checked on CentOS5/2.8.2, mingw, msys, msvs8 and msvs10.


2010/11/9 Óscar Fuentes <ofv at wanadoo.es>:
> 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.

Sure. I reconfirmed what I was wrong.
I have not realized that because I prefer "make edit_cache" :p

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

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

My intention is simply to separate simple options to list,
not to separate complex arguments.
It seems UNIX_COMMAND works fine.
I may choose whatever works. What would be better?

Without separation, we will see lit.py "-sv --no-progress-bar".

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

I intended to minimize changes and append LLVM_LIT_ARGS to tail of EXTRA_ARGS.
It makes sense not to use CLANG_TEST_EXTRA_ARGS.

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

I added an entry to docs/CMake.html. Feel free to add something or
rewrite whole.


...Takumi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-CMake-Add-the-new-option-LLVM_LIT_ARGS.patch
Type: application/octet-stream
Size: 2567 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20101110/647cf542/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-test-CMakeLists.txt-Use-LLVM_LIT_ARGS-and-remove-red.patch
Type: application/octet-stream
Size: 2599 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20101110/647cf542/attachment-0001.obj>