patch: improving the add_clang_unittest() CMake function, and using it to add a target for unittests/Basic

Hi, chapuni,

Would you please take a look at Issue 4183051: add a cmake target for unittests/Basic; also improve the API of add_clang_unittest() - Code Review
? The idea is to make "link components" and "used libs" arguments of
add_clang_unittest() as nature intends them to be. :wink: jyasskin
reviewed it, but we'd like to hear your opinion before committing it.
Thanks!

Hello Wan,

It works well on my hosts, thank you!

Two minor things. They might be trivial and one's preference.

  - You may create a list as semicolon-separated string.
    eg) "gtest;gtest_main;clangFrontend"
    Then, separate_arguments(string) would not be needed.
  - Could you split a patch into two?
    1. improvement
    2. the rule for BasicTests.

ps. It can be ported to llvm/unittests, too :slight_smile:

...Takumi

Hi Takumi,

It's ok. Please commit, thank you!

...Takumi

dgregor suggested CMake Wiki has moved for
making these arguments easier to read.

Interesting. I fear that it can be more confusing than helpful, as
there's no strong syntactical distinction between the parameter names
and their values -- they are all just a token in the argument list.

CMake uses the [KEYWORDS introduce a LIST of arguments] convention in
lots of other places, so I think people need to get over that
confusion anyway. It's definitely better than the system this patch
introduced, which caused Bigcheese to suggest that I remove a
positional argument when I reduced it to an empty string.

I think a space-separated string is easier to parse (for a human
reader) than a semicolon-separated one, as it makes the boundary of
each word very obvious, especially with monospace fonts. If you are
fine with it, I'd like to go with the current version.

dgregor suggested CMake Wiki has moved for
making these arguments easier to read.

Interesting. I fear that it can be more confusing than helpful, as
there's no strong syntactical distinction between the parameter names
and their values -- they are all just a token in the argument list.

CMake uses the [KEYWORDS introduce a LIST of arguments] convention in
lots of other places,

Then it's different. But why did CMake choose this hack over
supporting list literal arguments properly? :wink:

(It's not a question for you, I know.)

In general, I don't like mixing data and meta data. I think it will
be really confusing when some of the arguments happen to be in
UPPER_CASE (e.g. AST).

so I think people need to get over that
confusion anyway. It's definitely better than the system this patch
introduced, which caused Bigcheese to suggest that I remove a
positional argument when I reduced it to an empty string.

Perhaps we can use a mix of both approaches?

add_clang_unittest(Foo
  "LINK_COMPONENTS Core"
  "USE_LIBS Basic Frontend"
  path/to/XYZ.cpp)

You can always quote the arguments (“AST”) to make it more obvious. Besides, all built-in CMake commands have the same issue, and I’ve never actually seen it cause any problems.

This point of using the macro to parse keyword arguments is that it makes user-defined macros look and act a lot more like built-in CMake commands. Doing something that’s a little bit like CMake’s keyword arguments but slightly different would be more confusing.

  • Doug