phosek added inline comments.
================
Comment at: bolt/CMakeLists.txt:88
+ if(CMAKE_SYSROOT)
+ list(APPEND extra_args -DCMAKE_SYSROOT=${CMAKE_SYSROOT})
+ endif()
----------------
michaelplatings wrote:
> 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.
That documentation is inaccurate. `CMAKE_SYSROOT` can be set outside of the
toolchain file, and thus needs to be passed through as is frequently done in
LLVM, see for example
https://github.com/llvm/llvm-project/blob/7ca1bcbc6043b7ab4635f04746d1b9480960e384/llvm/cmake/modules/LLVMExternalProjectUtils.cmake#L261
or
https://github.com/llvm/llvm-project/blob/7ca1bcbc6043b7ab4635f04746d1b9480960e384/compiler-rt/cmake/Modules/AddCompilerRT.cmake#L650.
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
