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