[PATCH] D102084: Fix lld macho standalone build by including llvm/Config/llvm-config.h instead of llvm/Config/config.h
Mariusz Ceier via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri May 14 10:57:23 PDT 2021
mceier added a comment.
In D102084#2759574 <https://reviews.llvm.org/D102084#2759574>, @thakis wrote:
> Sorry about the delay here.
No problem ;)
================
Comment at: llvm/cmake/modules/LLVMConfig.cmake.in:114
+set(HAVE_LIBXAR "@HAVE_LIBXAR")
+
----------------
thakis wrote:
> While patching this in locally to land, I noticed that this doesn't have the trailing `@` that all other lines in this file have. Are you sure this does the right thing as-is?
>
> Also, everything in this file and almost everything in llvm-config.h starts with `LLVM_` which makes sense if the purpose of this file is to include it in external codebases. Maybe we should add an `LLVM_` prefix to this and the other symbol in this change? (But not sure about this part.)
> While patching this in locally to land, I noticed that this doesn't have the trailing @ that all other lines in this file have. Are you sure this does the right thing as-is?
Hmm; it probably is not right - if(HAVE_LIBXAR) will always be true in this case and always link with xar, so I missed one case when xar is not available on the system when testing.
> Maybe we should add an LLVM_ prefix to this and the other symbol in this change?
Ok, I can add the prefix.
Will send patches soon. Thanks for the review ;)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102084/new/
https://reviews.llvm.org/D102084
More information about the llvm-commits
mailing list