phosek accepted this revision.
phosek added a comment.
This revision is now accepted and ready to land.
LGTM
================
Comment at: clang/CMakeLists.txt:751
+ if(BOOTSTRAP_LLVM_ENABLE_LLD)
+ if(MSVC AND NOT BOOTSTRAP_CMAKE_SYSTEM_NAME)
+ set(${CLANG_STAGE}_LINKER
-DCMAKE_LINKER=${LLVM_RUNTIME_OUTPUT_INTDIR}/lld-link.exe)
----------------
krisb wrote:
> phosek wrote:
> > I don't understand the second part of this condition, can you elaborate?
> > Why not set `CMAKE_LINKER` to `lld-link.exe` even if
> > `BOOTSTRAP_CMAKE_SYSTEM_NAME STREQUAL "Windows"`?
> The only reason that stopped me from adding `BOOTSTRAP_CMAKE_SYSTEM_NAME
> STREQUAL "Windows"` case here is that there is no way to keep it working in
> the future (or, at least, I don't know one). Moreover, the build which sets
> BOOTSTRAP_CMAKE_SYSTEM_NAME equal to "Windows" is currently broken.
> Since the same issue is exposed if to build LLVM with
> `clang/cmake/caches/DistributionExample.cmake` on Windows, I included only
> the changes that contribute to making this configuration buildable. I wanted
> to avoid making the impression that some other configurations are supported
> (because they are not) and also avoid adding any dead code that nobody would
> use and that would easily be broken.
>
I'd prefer to use `WIN32 OR BOOTSTRAP_CMAKE_SYSTEM_NAME STREQUAL "Windows"`
here even if we don't support cross-compilation to Windows now as I hope it's
something we're going to support in the future and this will be one less thing
we need to address. Alternatively, please at least leave a `TODO` so it's
easier to find later.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80873/new/
https://reviews.llvm.org/D80873
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
