[PATCH] D46288: Don't redefine a bunch of defines from llvm-config.h in config.h.

[Nico Weber via Phabricator via llvm-commits]

thakis added inline comments.


================
Comment at: include/llvm/Config/config.h.cmake:296-297
-/* Target triple LLVM will generate code for by default */
-/* Doesn't use `cmakedefine` because it is allowed to be empty. */
-#define LLVM_DEFAULT_TARGET_TRIPLE "${LLVM_DEFAULT_TARGET_TRIPLE}"
-
----------------
bogner wrote:
> thakis wrote:
> > thakis wrote:
> > > bogner wrote:
> > > > The one in llvm-config.h doesn't have this comment and does use cmakedefine. Please make sure we're still doing the right thing when this is empty (that is, build with -DLLVM_DEFAULT_TARGET_TRIPLE="")
> > > Since config.h includes llvm-config.h, when it's undefined in config.h the one in llvm-config.h won. So this is behavior-preserving, and since this doesn't work it looks like the comment is outdated.
> > Did this make sense?
> No, I think you have this backwards. Today, if LLVM_DEFAULT_TARGET_TRIPLE is blank, llvm-config.h has a comment like so:
> 
>   /* #undef LLVM_DEFAULT_TARGET_TRIPLE */
> 
> Whereas config.h defines the value to an empty string:
> 
>   #define LLVM_DEFAULT_TARGET_TRIPLE ""
> 
> So removing this from config.h will make it so that it's neither #defined nor #undef'd in either file, which is not behaviour preserving. In fact, if you build this way you'll probably get a compile error in lib/Support/Unix/Host.inc when it tries to call updateTripleOSVersion with no parameter. This is what I was asking you to check here.
> 
> Moving the explicit define from here to llvm-config.h is also technically not behaviour preserving, since external projects that use these headers will see a #define of "" instead of no symbol at all, but I think that's at least a change that is arguably correct.
> 
Got it, thanks! I removed LLVM_DEFAULT_TARGET_TRIPLE from this change.


https://reviews.llvm.org/D46288