michaelplatings added inline comments.
================
Comment at: bolt/CMakeLists.txt:88
+ if(CMAKE_SYSROOT)
+ list(APPEND extra_args -DCMAKE_SYSROOT=${CMAKE_SYSROOT})
+ endif()
----------------
The [[ https://cmake.org/cmake/help/latest/variable/CMAKE_TOOLCHAIN_FILE.html |
documentation for CMAKE_SYSROOT ]] says "This variable may only be set in a
toolchain file specified by the CMAKE_TOOLCHAIN_FILE variable."
Maybe pass CMAKE_TOOLCHAIN_FILE through instead?
================
Comment at: bolt/CMakeLists.txt:88
+ if(CMAKE_SYSROOT)
+ list(APPEND extra_args -DCMAKE_SYSROOT=${CMAKE_SYSROOT})
+ endif()
----------------
michaelplatings wrote:
> The [[ https://cmake.org/cmake/help/latest/variable/CMAKE_TOOLCHAIN_FILE.html
> | documentation for CMAKE_SYSROOT ]] says "This variable may only be set in
> a toolchain file specified by the CMAKE_TOOLCHAIN_FILE variable."
>
> Maybe pass CMAKE_TOOLCHAIN_FILE through instead?
`list(APPEND variable_that_may_or_may_not_exist ...` is quite fragile. Should
someone have `-Dextra_args=X` on their cmake command line then that will be
included here, which I don't think is the intent. I suggest adding
`set(extra_args "")` above the `if`. This also improves readability because
otherwise you have to scan the whole file to figure out that `extra_args`
wasn't previously defined.
================
Comment at: bolt/CMakeLists.txt:98
-DCMAKE_MAKE_PROGRAM=${CMAKE_MAKE_PROGRAM}
- -DCMAKE_INSTALL_PREFIX=${LLVM_BINARY_DIR}
+ -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}
-DLLVM_LIBDIR_SUFFIX=${LLVM_LIBDIR_SUFFIX}
----------------
Given that the install step now does nothing, and the `install` statement below
passes `-DCMAKE_INSTALL_PREFIX` explicitly, could this line be deleted?
Alternatively perhaps `-DCMAKE_INSTALL_PREFIX` could be removed from the
`install` statement.
================
Comment at: bolt/CMakeLists.txt:103
+ INSTALL_COMMAND ""
+ STEP_TARGETS configure build
BUILD_ALWAYS True
----------------
These targets don't appear to be used. I suggest either deleting this line, or
if they're needed/useful somehow then adding a comment either here or in the
commit message.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150752/new/
https://reviews.llvm.org/D150752
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
