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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits