[PATCH] D141050: [standalone-build] outsource, simplify and unify repetitive CMake code

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.


================
Comment at: clang/CMakeLists.txt:35
+  include(StandaloneBuildHelpers)
+  get_llvm_utility_binary_path("llvm-tblgen" "LLVM_TABLEGEN_EXE")
 
----------------
It's somewhat unusual to quote output variable names in our CMake files, I'd prefer to follow existing conventions for consistency.


================
Comment at: cmake/Modules/StandaloneBuildHelpers.cmake:12-19
+if (NOT (CLANG_BUILT_STANDALONE
+         OR LLD_BUILT_STANDALONE
+         OR COMPILER_RT_STANDALONE_BUILD
+         OR OPENMP_STANDALONE_BUILD
+         OR MLIR_STANDALONE_BUILD
+         OR LLDB_BUILT_STANDALONE))
+    message(FATAL_ERROR "Make sure you build in standalone mode")
----------------
I'd omit this, I don't think it's necessary to restrict the usage of this module.


================
Comment at: cmake/Modules/StandaloneBuildHelpers.cmake:25-30
+if(NOT MSVC_IDE)
+set(LLVM_ENABLE_ASSERTIONS ${ENABLE_ASSERTIONS}
+  CACHE BOOL "Enable assertions")
+# Assertions should follow llvm-config's.
+mark_as_advanced(LLVM_ENABLE_ASSERTIONS)
+endif()
----------------
Do you know why is this needed or this just copy pasted from Clang? I'd consider dropping this altogether (ideally in a separate change).


================
Comment at: cmake/Modules/StandaloneBuildHelpers.cmake:45-46
+
+# Automatically add the current source and build directories to the include path.
+set(CMAKE_INCLUDE_CURRENT_DIR ON)
+
----------------
This shouldn't be needed since you've already had to insert the module path manually.


================
Comment at: cmake/Modules/StandaloneBuildHelpers.cmake:61
+function(get_llvm_utility_binary_path utility out_var)
+    set(_imploc IMPORTED_LOCATION_NOCONFIG)
+    # Based on the build type that the installed LLVM was built with,
----------------
We use 2 spaces for indentation in our CMake files.


================
Comment at: cmake/Modules/StandaloneBuildHelpers.cmake:82-109
+# The set_lit_defaults macro exists because the code existed in multiple
+# locations before.
+macro(set_lit_defaults)
+    set(LIT_ARGS_DEFAULT "-sv")
+    if (MSVC OR XCODE)
+      set(LIT_ARGS_DEFAULT "${LIT_ARGS_DEFAULT} --no-progress-bar")
+    endif()
----------------
Is there any reason to keep these two macros separate? Could we combine them?


================
Comment at: lld/CMakeLists.txt:57
+    set_lit_defaults()
+    get_llvm_utility_binary_path("FileCheck" "FileCheck_EXE")
+    get_llvm_utility_binary_path("count" "count_EXE")
----------------
Where are these variables being used?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141050