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

[Justin Bogner via Phabricator via llvm-commits]

bogner 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}"
-
----------------
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.



https://reviews.llvm.org/D46288