[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

+1, I think this is an improvement, it looks good overall, some nits.



================
Comment at: clang-tools-extra/CMakeLists.txt:49
+endif()
+
----------------
nit: remove the extra blank line.


================
Comment at: clang/CMakeLists.txt:196
+    umbrella_lit_testsuite_begin(check-all)
   endif()
 
----------------
nit: add a `# LLVM_INCLUDE_TESTS`, this is a deeply-nested if statement, it would be easier to spot which if does the `umbrella_lit_testsuite_begin` stay.


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:1827
 
+# Convert a target name like check-clangd to a variable name like CLANG.
+function(umbrella_lit_testsuite_var target outvar)
----------------
if I read the code correctly, input `check-clangd` the output is CLANGD, not CLANG.


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:1876
+    get_property(gather_names GLOBAL PROPERTY LLVM_LIT_UMBRELLAS)
+    set_property(GLOBAL APPEND PROPERTY LLVM_LIT_UMBRELLAS ${name})
+    foreach(name ${gather_names})
----------------
I don't know where is the `name` variable? it seems at this point we don't have a name var (unlike the code in the `foreach(name ${gather_names})` below), I guess we probably should move it to the `foreach`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121838/new/

https://reviews.llvm.org/D121838